ComposeMenu.js: Handle null for fileName in response from ImagePicker. #3818
Conversation
src/compose/ComposeMenu.js
Outdated
| @@ -63,7 +63,7 @@ class ComposeMenu extends PureComponent<Props> { | |||
| // https://github.com/react-native-community/react-native-image-picker/blob/master/docs/Reference.md | |||
| error?: string | void | null | false, | |||
| uri: string, | |||
| fileName: string, | |||
| fileName: ?string, | |||
There was a problem hiding this comment.
This shouldn't have typechecked anyway – this isn't a supertype of Response!
Flow isn't catching this because of the $FlowFixMe up top: ImagePicker is defined as any. Changing the import to import * as ImagePicker (and removing the suppression comment) lets Flow see the definitions.
It seems that either import method works, at least on Android. Which is surprising, to say the least – but perhaps there's some React linkage magic going on. Does it work both ways on iOS?
There was a problem hiding this comment.
Hmm, I think I see what you mean -- using import * as ImagePicker reveals that the callback we're passing, this.handleImagePickerReponse, isn't accepted by Flow because of the types specified by react-native-image-picker (specifically, .launchCamera expects to pass a Response to that callback, which doesn't include fileName being null, and ours does). I remember being vaguely surprised that no Flow errors were introduced when I made this change, and that I didn't have to add any suppressions. I should probably have at least made a note of my surprise there. Turns out it's because of this very general suppression, as you point out.
I wonder if we can avoid doing such a general suppression. If you get a moment, would you like to go over this in person? I've poked around a bit here and I'm not sure how best to use a modified Response type and a modified type for ImagePicker.launchCamera without making a local copy of react-native-image-picker, which would complicate staying up to date with newer versions. (Maybe we'd just need to make that local copy anyway.)
chrisbobbe
force-pushed
the
issue-3813
branch
from
2fa20b7 to
22931ca
Compare
|
OK, I've updated the pull request to reflect our chat earlier. |
|
Haven't tested yet, but looks good codewise.
|
Indeed, and I should know 🤣. I have been meaning to configure Nano to do this for me automatically, but I wasn't sure what any available shortcuts might be to ease enforcing different limits on the summary and the body; I'll ping you or Greg tomorrow on this, and address your other comments. Thank you! |
Remove $FlowFixMe above import of ImagePicker; change to `import * as ImagePicker` so ImagePicker is no longer treated as `any`. This was hiding an error caused by the `response` param in this.handleImagePickerResponse not being $ReadOnly, so, make it $ReadOnly so no inconsistencies can be introduced by modifying it. Also revealed was the need to type `fileName` to string | void to match the Response type in react-native-image-picker; do.
Fixes: zulip#3813. On iOS, it appears that the fileName is not included in the response given by ImagePicker.launchCamera if the user has not granted read access to their photo library. The case where fileName is undefined was handled correctly, but it turns out it's coming through as null, which was not handled, so, handle null. Neither the types nor the docs from react-native-image-picker are clear on this point, so that was filed as react-native-image-picker/react-native-image-picker#1271. Do this without creating a local copy of react-native-image-picker with our own, correct type, since it's very likely that we'll need to upgrade to later versions frequently as we're on an early version of this library which is known to have limitations and bugs. Add tests for fileName being undefined and being null.
chrisbobbe
force-pushed
the
issue-3813
branch
from
22931ca to
ed1080c
Compare
|
OK, this should be ready for another review, when convenient. 🙂 |
|
✔️ |
|
Thanks for the review and merge! |
Fixes #3813.