deps: Take all minor upgrades; ~compileSdkVersion 33;~ camera-roll 5.x #5543
Conversation
|
(Bump, now that I've properly filled in the title and description, for the sake of causing email notifications with a better subject line.) |
808eefb to
b8ba141
Compare
There was a problem hiding this comment.
Thanks!
I've just tested this on iOS (my iPhone 13 Pro, running iOS 16.1).
The react-native-image-picker upgrade seems problematic, unfortunately. Here's some background:
- For a while, we've had a problem (tracked in the stalled PR #4475) where, for image uploads on iOS, the app sends a copy of the original image with a much-increased size.
- ISTR at one point we diagnosed the problem as follows: Apple's API for making a GIF copy (
UIImageJPEGRepresentation) gives a result with a much-increased size when you ask for quality 1.0. The image was being passed through this API, with quality 1.0, twice: once inreact-native-image-pickerand once inreact-nativeinternals. - We've hoped that facebook/react-native@f78526ce3, released in RN v0.66, would relieve the symptom by removing the
UIImageJPEGRepresentationcall in RN. We haven't tested that hypothesis; I hope it removed the call but I don't know yet. - I've been vaguely optimistic that the burn-it-down rewrite of
react-native-image-pickerin v3 might've completed the fix by removing its call. I think that hasn't happened?
The react-native-image-picker upgrade in your PR gives us react-native-image-picker/react-native-image-picker#2006, "fix: the fileSize of the selected image is bigger than the origin one in ios", released in 4.8.5. Sounds promising… 🤔
But our app's problem hasn't gone away. At the tip of this PR branch, having run yarn,
- I tested the upload-from-library flow. The image was taken last Friday by my iPhone, and its detail page in my Photos app claims it's 3.5MB. When it finished uploading, I downloaded it from the Zulip web-app lightbox, and it's showing as 8.1MB in Finder.
- I tested the upload-from-camera flow. The image was saved to my iPhone; its detail page in my Photos app claims it's 1.4MB. When it finished uploading, I downloaded it from the Zulip web-app lightbox, and it's showing as 3.7MB in Finder. Also—and this is consistent with react-native-image-picker/react-native-image-picker#2022, which blames the 4.8.5 upgrade—the image was rotated 90° counterclockwise.
I didn't test on main to compare, but the 90°-rotation bug seems new and something we'd want to avoid.
Here are things I did that succeeded. (I used GitHub's "rich diff" feature for viewing all the yarn.lock changes from this PR, and went down the list. Helpful UI! 🙂)
- I successfully built and ran the app
- The upgraded
@react-native-camera-roll/camera-rollstill seems to work; I successfully downloaded an image from the lightbox - The upgraded
react-native-gesture-handlerstill seems to work (at least when not in "Debug with Chrome" mode; gestures are known not to work in that mode):- Swipe-right-from-left to go back
- Swipe between top tabs in the see-who's-reacted screen
- The upgraded
@expo/react-native-action-sheetstill seems to work; I was able to open the action sheet and view read receipts for a message. - The upgraded
expo-apple-authenticationstill seems to work; I successfully signed in with Apple. - The upgraded
react-native-safe-area-contextstill seems to work; I didn't notice new bugs with the safe-area insets, even when I rotated to landscape mode - I didn't see any new bugs that would point to something wrong in
react-native-screens, but let's keep an eye out because the changelog lists a lot of stuff between 3.13.1 and 3.18.2. - The upgraded
react-native-vector-iconsstill seems to work; didn't notice any wrong-looking icons. - The upgraded
react-native-webviewstill seems to work; didn't notice anything wrong in the webview. - Changes to
react-redux,redux-thunk, andreselectseem small from their changelogs, and I didn't see anything wrong with these while testing all the other stuff.
Things I didn't test:
- Sentry; I hope we'd notice anything gone wrong in the next beta release
Other things I happened to notice:
- If we were using
tsflowerforexpo-sqlite, we'd automatically get a type adjustment that would support usingnullas a query argument: expo/expo#18078
| static getAlbums(params?: GetAlbumsParams): Promise<Album[]>; | ||
| static getParamsWithDefaults(params: GetPhotosParams): GetPhotosParams; | ||
| static getPhotos(params: GetPhotosParams): Promise<PhotoIdentifiersPage>; | ||
| static iosGetImageDataById(internalID: string, convertHeicImages?: boolean): Promise<PhotoIdentifier>; |
There was a problem hiding this comment.
tools/test prettier isn't happy with the length of this line, probably because it got longer in the patch that added static to it.
There was a problem hiding this comment.
Hmm, indeed. Puzzling that it passed CI, then.
Ah, I see. It's that in the tools/test --all case, the prettier suite relies on this line:
patterns=( "${@/%\///**/*.js}" ) # replace trailing `/` with `/**/*.js`
And that line wasn't adapted to reflect our wanting to cover .js.flow files as well as .js files.
I'll see about fixing that too, perhaps as a separate PR.
android/build.gradle
Outdated
| @@ -19,7 +19,7 @@ buildscript { | |||
| // https://medium.com/androiddevelopers/picking-your-compilesdkversion-minsdkversion-targetsdkversion-a098a0341ebd | |||
| // What's the latest? Consult this list: | |||
| // https://developer.android.com/studio/releases/platforms | |||
| compileSdkVersion = 31 | |||
| compileSdkVersion = 33 | |||
There was a problem hiding this comment.
android build: Bump compileSdkVersion to 33, aka Android 13
At this commit, tools/test native failed for me with this:
/Users/chrisbobbe/dev/zulip-mobile/node_modules/react-native-image-picker/android/src/main/java/com/imagepicker/VideoMetadata.java:46: error: unreported exception IOException; must be caught or declared to be thrown
metadataRetriever.release();
^
That error looks like it matches react-native-image-picker/react-native-image-picker#2013, which links to PR react-native-image-picker/react-native-image-picker#2003, which links to a PR react-native-image-picker/react-native-image-picker#1955 that was merged and reportedly released in react-native-image-picker 4.9.0.
I see that the main yarn upgrade commit would take us to 4.10.0. Do you reproduce this error at this commit, and if so, would it make sense to bump react-native-image-picker in this commit or before it, to avoid this error?
There was a problem hiding this comment.
Hrm, I guess this is in conflict with my finding in #5543 (review) that 4.8.5+ is problematic. 😵
There was a problem hiding this comment.
At this commit,
tools/test nativefailed for me with this:
Hmm, good catch, thanks. I thought I'd checked that sort of thing, but apparently missed this.
That error looks like it matches react-native-image-picker/react-native-image-picker#2013, which links to PR react-native-image-picker/react-native-image-picker#2003, which links to a PR react-native-image-picker/react-native-image-picker#1955 that was merged and reportedly released in
react-native-image-picker4.9.0.
Ugh. Looks like they fixed the error making it possible to build on the new SDK:
https://github.com/react-native-image-picker/react-native-image-picker/pull/1955/files#diff-68f6dace32eaf1f8c9e342c552cb4984338d3e185ccb1552d72fbb5df278f77e
in the same commit where they also added that feature that made it necessary to do so. Bad choice.
Well, I think this was also the one thing that made it necessary to bump the compileSdkVersion. So I may be able to get a working upgrade branch by:
- holding back react-native-image-picker to 4.8.4, before both this change and the preceding one that introduced that rotation bug;
- and holding back compileSdkVersion at 31.
I'll go try that now. Then if that works, straightening things out on react-native-image-picker and doing the compileSdkVersion bump can be a follow-up.
There was a problem hiding this comment.
straightening things out on react-native-image-picker and doing the compileSdkVersion bump can be a follow-up.
| | 'blocked' | ||
| | 'not-determined'; | ||
| -declare export var cameraRollEventEmitter: NativeEventEmitter; | ||
| +declare export var cameraRollEventEmitter: NativeEventEmitter<{| +[string]: mixed |}>; |
There was a problem hiding this comment.
gnprice/tsflower#1 is open to add NativeEventEmitter to tsflower's subst/react-native.js.flow; I've just posted a status update there: gnprice/tsflower#1 (comment)
That doesn't need to block this quick-and-easy patch, of course, but it'll be nice to get in eventually, I think.
| override fun createReactActivityDelegate(): ReactActivityDelegate { | ||
| return ReactActivityDelegateWrapper( | ||
| this, | ||
| object : ReactActivityDelegate(this, mainComponentName) { | ||
| override fun createRootView(): ReactRootView { | ||
| return RNGestureHandlerEnabledRootView(this@MainActivity) | ||
| } | ||
| } | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
This diff, which I see comes from that react-native-gesture-handler doc you linked to, conflicts with how the Expo template app handles the same API change. In particular, see discussion, where I link to expo/expo@a2dc4da94, which keeps the createReactActivityDelegate override but removes the call to RNGestureHandlerEnabledRootView.
Hmm, in fact: that Expo change might not be at odds with the r-n-gesture-handler doc you linked to. Emphasis mine:
Update your
MainActivity.javafile (or wherever you create an instance ofReactActivityDelegate), so that it no longer overrides the method responsible for creatingReactRootViewinstance, or modify it so that it no longer usesRNGestureHandlerEnabledRootView.
It might be good to follow Expo here, if possible, since later Expo versions propose changes based on the method override still being there: https://github.com/expo/expo/blame/main/templates/expo-template-bare-minimum/android/app/src/main/java/com/helloworld/MainActivity.java
There was a problem hiding this comment.
Ah indeed, thanks. I had the feeling I'd seen a change in that direction go by, and didn't succeed in locating it -- that's the one.
Looks like expo/expo@dec397f is that later Expo change, in Expo 46.
One thing I notice now is that that ReactActivityDelegateWrapper is an Expo thing:
import expo.modules.ReactActivityDelegateWrapper
So there may even be something useful it's doing even in the expo/expo@a2dc4da94 version where the createRootView override in its argument is gone.
| import { type NativeSafeAreaViewProps } from './SafeArea.types'; | ||
| import NativeSafeAreaView from './specs/NativeSafeAreaView'; | ||
| -type NativeSafeAreaViewInstance = InstanceType<typeof NativeSafeAreaView>; | ||
| +type NativeSafeAreaViewInstance = $Call<typeof NativeSafeAreaView>; |
There was a problem hiding this comment.
Would it be wrong to use React's ElementRef type instead of $Call here?
There was a problem hiding this comment.
Hmm. Yeah, on second look $Call isn't right at all. That was an attempt to translate TS's InstanceType -- but InstanceType takes a constructor to the type it returns called as a constructor, with new, which isn't the same as the normal function call that $Call represents.
Unpacking the definition of NativeSafeAreaView, its type is a React.AbstractComponent<…>. So yeah, I think React.ElementRef is appropriate.
Ugh. Fun. Thanks for finding this. There's that RN issue that also caused this sort of bloat. It looks like the commit you mentioned was meant to fix this issue: For the rotation: someone kindly sent a PR to fix it: |
|
Thanks for the review and the thorough testing! Replied on the react-native-image-picker issue just above; I'll look at your other comments tomorrow, and perhaps see if I can produce a version that holds that back in order to unblock the rest. |
b8ba141 to
2813b21
Compare
|
OK, updated! I held back react-native-image-picker at 4.8.4 (our current version, and the last one before that rotation bug), and then held back compileSdkVersion at 31 as a result, as per this subthread: #5543 (comment) Also made smaller changes discussed above. Marked as non-draft thanks to your testing. |
This also means following a rename, from the former `@react-native-community/cameraroll`. We had had our own small patch to this library, making the Flow types in its implementation compatible with modern Flow practice: react-native-cameraroll/react-native-cameraroll#328 In v5, that became obsolete because they converted the implementation from Flow to TypeScript. So just drop the patch. That also means we're losing type-checking on the library's interface. We'll get types back with TsFlower in the next commit; for now just leave a fixme. We use very little API surface area here, so it's not essential to have types on it.
This is needed in order to bump compileSdkVersion to 33 without hitting this build error: software-mansion/react-native-gesture-handler#2096 The upgrade requires a change in our Android MainActivity class, like so: https://docs.swmansion.com/react-native-gesture-handler/docs/guides/migrating-off-rnghenabledroot/ For the details of that change, we follow Expo's template app: expo/expo@a2dc4da94 Discussion here: zulip#5543 (comment)
Starting in version 7.31.9 of eslint-plugin-react, this rule has a bug where it talks about `contextTypes` when in reality we don't have and never want a `contextTypes`: jsx-eslint/eslint-plugin-react#3467 Just disable the rule -- it's not a valuable enough rule to spend effort hacking around the bug.
The next release, 4.8.5, introduces a bug where images get rotated: zulip#5543 (comment) There's an open PR to fix the issue: react-native-image-picker/react-native-image-picker#2036 So we might switch to that PR version until the fix goes upstream. For the moment, just hold this dependency back so that `yarn upgrade` can cleanly upgrade everything else.
2813b21 to
36a03ec
Compare
|
Thanks, LGTM! Merged. |
This is needed in order to bump compileSdkVersion to 33 without hitting this build error: software-mansion/react-native-gesture-handler#2096 The upgrade requires a change in our Android MainActivity class, like so: https://docs.swmansion.com/react-native-gesture-handler/docs/guides/migrating-off-rnghenabledroot/ For the details of that change, we follow Expo's template app: expo/expo@a2dc4da94 Discussion here: zulip#5543 (comment)
The next release, 4.8.5, introduces a bug where images get rotated: zulip#5543 (comment) There's an open PR to fix the issue: react-native-image-picker/react-native-image-picker#2036 So we might switch to that PR version until the fix goes upstream. For the moment, just hold this dependency back so that `yarn upgrade` can cleanly upgrade everything else.
….10.2 Seems like react-native-image-picker made it *possible* to build on the new SDK in the same commit where they made it *necessary* to do so, which was a bad choice: zulip#5543 (comment) So, shove the sdk bump and this image-picker upgrade together into one commit. Anyway, as Greg said in his draft of the compileSdkVersion bump, f5c90ec: This version is out; time to start using it in the build. This setting is not to be confused with the targetSdkVersion. The latter goes into the built manifest, and affects a wide range of behavior, so bumping it requires careful testing. This is used purely at build time, and should have no effect on runtime behavior. Its main effect is that it become possible for code to conditionally use new API features. It also brings new compiler warnings -- hence the pair of library updates preceding this. We take the latest image-picker version, but in particular at least 4.10.1, to get a bugfix for an image-orientation issue: react-native-image-picker/react-native-image-picker#2036 that was reportedly introduced in 4.8.5: react-native-image-picker/react-native-image-picker#2022
….10.2 Seems like react-native-image-picker made it *possible* to build on the new SDK in the same commit where they made it *necessary* to do so, which was a bad choice: zulip#5543 (comment) So, shove the sdk bump and this image-picker upgrade together into one commit. Anyway, as Greg said in his draft of the compileSdkVersion bump, f5c90ec: This version is out; time to start using it in the build. This setting is not to be confused with the targetSdkVersion. The latter goes into the built manifest, and affects a wide range of behavior, so bumping it requires careful testing. This is used purely at build time, and should have no effect on runtime behavior. Its main effect is that it become possible for code to conditionally use new API features. It also brings new compiler warnings -- hence the pair of library updates preceding this. We take the latest image-picker version, but in particular at least 4.10.1, to get a bugfix for an image-orientation issue: react-native-image-picker/react-native-image-picker#2036 that was reportedly introduced in 4.8.5: react-native-image-picker/react-native-image-picker#2022
The final commit here runs
yarn upgrade. The others represent changes that were needed in order to unblock that.Specifically, the new react-native-image-picker version requires
compileSdkVersionto be bumped to 33, the newest: react-native-image-picker/react-native-image-picker#2018 (comment)That in turn requires upgrading react-native-gesture-handler beyond the
~constraint we'd had on it, and requires upgrading react-native-camera-roll from 4.x to 5.0.2:react-native-cameraroll/react-native-cameraroll#429
@chrisbobbe would you do some manual testing of this PR on iOS? I've done so on Android… but the app's iOS build seems to be broken on my Mac, and I've spent a couple of hours on that so far today without success. I'll start a separate conversation (edit: here) on that, but in the meantime figure it's better to unblock this.