Migrate from Unimodules to Expo Modules #5203
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This'll be too long for a commit message, so I'll leave it here. We use five Collapsed below are the details of how I plan to upgrade those five modules, in the same commit that switches us over from `expo-web-browser`expo-web-browser seemed to make the switch at 9.3.0:
`expo-application`expo-application seemed to make the switch at 3.3.0:
`expo-apple-authentication`expo-apple-authentication seemed to make the switch at 4.0.0:
`expo-sqlite`expo-sqlite seemed to make the switch at 10.0.0:
`expo-screen-orientation`expo-screen-orientation seemed to make the switch at 4.0.0:
|
|
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Jan 25, 2022
Done by mostly following the changes to Expo's templates/expo-template-bare-minimum/ in expo/expo@9781212eb. Expo's description of the new infrastructure is at https://blog.expo.dev/whats-new-in-expo-modules-infrastructure-7a7cdda81ebc . They gave a migration guide that suggested making changes similar to these, but it didn't end up helping us understand why they were the right changes to make or what would happen if we made them. That guide is at https://github.com/expo/fyi/blob/main/expo-modules-migration.md . We've cleared up some of the mystery; see zulip#5203 (comment) and previous commits in this series. In this commit: - (Mostly follow expo/expo@9781212eb, as mentioned) - Also, upgrade all our `expo-*` direct dependencies so that they work with the new system. (We upgrade them minimally, to minimize having to think about possible unrelated breaking changes right now.) Details at zulip#5203 (comment) . - Remove android/app/src/main/java/com/zulipmobile/generated/BasePackageList.java, since its only import was removed. (Expo didn't have this file in version control, and the migration guide didn't mention the file.) - The Expo commit assumes that our project has done special setup for `expo-updates`, `expo-constants`, and `expo-splash-screen`. It makes changes to that setup, which we ignore since we don't use any of those. If we need them in the future, we'll just look up their current setup instructions at the time. - Don't add an empty Swift file. The migration guide says, "A blank Swift file must be created for native modules with Swift files to work correctly." With `find node_modules | grep .swift` in our project, I see that expo-modules-core and expo-web-browser have many Swift files in them, and I don't have any problems building or running the app without an empty Swift file. The template app doesn't add one either. - Don't add a Podfile.properties.json file. Like the changes in expo/expo@dbd384b22, this would have us put certain config values in some new Expo-branded variables. We're happy keeping them inline. - Update our value for `transformIgnorePatterns` in the Jest config, following an error when running Jest. Fixes: zulip#5133
chrisbobbe
force-pushed
the
pr-expo-modules
branch
from
577134a
to
1c7941e
Compare
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Jan 25, 2022
Done by mostly following the changes to Expo's templates/expo-template-bare-minimum/ in expo/expo@9781212eb. Expo's description of the new infrastructure is at https://blog.expo.dev/whats-new-in-expo-modules-infrastructure-7a7cdda81ebc . They gave a migration guide that suggested making changes similar to these, but it didn't end up helping us understand why they were the right changes to make or what would happen if we made them. That guide is at https://github.com/expo/fyi/blob/main/expo-modules-migration.md . We've cleared up some of the mystery; see zulip#5203 (comment) and previous commits in this series. In this commit: - (Mostly follow expo/expo@9781212eb, as mentioned) - Also, upgrade all our `expo-*` direct dependencies so that they work with the new system. (We upgrade them minimally, to minimize having to think about possible unrelated breaking changes right now.) Details at zulip#5203 (comment) . - Remove android/app/src/main/java/com/zulipmobile/generated/BasePackageList.java, since its only import was removed. (Expo didn't have this file in version control, and the migration guide didn't mention the file.) - The Expo commit assumes that our project has done special setup for `expo-updates`, `expo-constants`, and `expo-splash-screen`. It makes changes to that setup, which we ignore since we don't use any of those. If we need them in the future, we'll just look up their current setup instructions at the time. - Don't add an empty Swift file. The migration guide says, "A blank Swift file must be created for native modules with Swift files to work correctly." With `find node_modules | grep .swift` in our project, I see that expo-modules-core and expo-web-browser have many Swift files in them, and I don't have any problems building or running the app without an empty Swift file. The template app doesn't add one either. - Don't add a Podfile.properties.json file. Like the changes in expo/expo@dbd384b22, this would have us put certain config values in some new Expo-branded variables. We're happy keeping them inline. - Update our value for `transformIgnorePatterns` in the Jest config, following an error when running Jest. - Some sub-dependency looks like it's changing the output of `generate-webview-js`. Examine that output (looks OK), and commit. Fixes: zulip#5133
chrisbobbe
force-pushed
the
pr-expo-modules
branch
from
1c7941e
to
6c20a41
Compare
chrisbobbe
force-pushed
the
pr-expo-modules
branch
5 times, most recently
from
7118abb
to
8170e1a
Compare
gnprice
reviewed
Jan 27, 2022
android/app/build.gradle
Outdated
Comment on lines
116
to
120
| apply from: new File( | ||
| ["node", "--print", "require.resolve('react-native/package.json')"] | ||
| .execute(null, rootDir).text.trim(), | ||
| "../react.gradle" | ||
| ) |
There was a problem hiding this comment.
Hmm, I'm not a fan of shelling out to node --print all over like this. In particular it's slow, because node is slow to start up:
$ time node --print "require.resolve('react-native/package.json')"
/home/greg/z/mobile/node_modules/react-native/package.json
time: 0.068s wall (0.049s u, 0.021s s)
So doing that eight times in the Android build config will add a big chunk of a second, which is sizable when you just want to run a quick unit test or something.
I believe that's not the worst offense that Expo has committed against decent build times -- IIRC doing anything with Gradle got several seconds slower when we first started using unimodules. Still, I'd rather leave it out if there isn't a pressing problem it solves for us.
(And if we were going to have this behavior, I'd definitely want to at least find a way to write it less messily, like with a little helper function. This version gets hard to read and error-prone.)
Previous discussion here:
#4949 (comment)
android/app/build.gradle
Outdated
Comment on lines
93
to
113
| // These three (cliPath, hermesCommand, composeSourceMapsPath) were | ||
| // added to templates/template-expo-bare-minimum/android in | ||
| // expo/expo@9781212eb, expo/expo@87a3d02aa, and | ||
| // expo/expo@c75c6c417. I think they're added to help with monorepo | ||
| // support (like expo/expo@05f2ed0c7), which we don't need. In this | ||
| // case, Expo's solution was to override some bad defaults in React | ||
| // Native: | ||
| // https://github.com/facebook/react-native/blob/v0.64.3/react.gradle#L38-L46 | ||
| // As of 2022-01-24, those defaults are unchanged in `main`. | ||
| cliPath : new File( | ||
| ["node", "--print", "require.resolve('react-native/package.json')"] | ||
| .execute(null, rootDir).text.trim(), | ||
| ).getParentFile().getAbsolutePath() + "/cli.js", | ||
| hermesCommand : new File( | ||
| ["node", "--print", "require.resolve('hermes-engine/package.json')"] | ||
| .execute(null, rootDir).text.trim() | ||
| ).getParentFile().getAbsolutePath() + "/%OS-BIN%/hermesc", | ||
| composeSourceMapsPath: new File( | ||
| ["node", "--print", "require.resolve('react-native/package.json')"] | ||
| .execute(null, rootDir).text.trim() | ||
| ).getParentFile().getAbsolutePath() + "/scripts/compose-source-maps.js", |
There was a problem hiding this comment.
Yeah, what a mess. Let's skip this.
If we ever put our code in a different directory structure where node_modules is a different number of levels up, we can fix all our existing references (like the ones the previous commit is about) with pretty much just perl -i -0pe 's</node_modules></../node_modules>g' -- $(git ls-files). And we can add a few lines that similarly configure these three values with another ../ (or whatever's needed) beyond the defaults.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Jan 28, 2022
Done by mostly following the changes to Expo's templates/expo-template-bare-minimum/ in expo/expo@9781212eb. Expo's description of the new infrastructure is at https://blog.expo.dev/whats-new-in-expo-modules-infrastructure-7a7cdda81ebc . They gave a migration guide that suggested making changes similar to these, but it didn't end up helping us understand why they were the right changes to make or what would happen if we made them. That guide is at https://github.com/expo/fyi/blob/main/expo-modules-migration.md . We've cleared up some of the mystery; see zulip#5203 (comment) and previous commits in this series. In this commit: - (Mostly follow expo/expo@9781212eb, as mentioned) - Also, upgrade all our `expo-*` direct dependencies so that they work with the new system. (We upgrade them minimally, to minimize having to think about possible unrelated breaking changes right now.) Details at zulip#5203 (comment) . - Remove android/app/src/main/java/com/zulipmobile/generated/BasePackageList.java, since its only import was removed. (Expo didn't have this file in version control, and the migration guide didn't mention the file.) - The Expo commit assumes that our project has done special setup for `expo-updates`, `expo-constants`, and `expo-splash-screen`. It makes changes to that setup, which we ignore since we don't use any of those. If we need them in the future, we'll just look up their current setup instructions at the time. - Don't add an empty Swift file. The migration guide says, "A blank Swift file must be created for native modules with Swift files to work correctly." With `find node_modules | grep .swift` in our project, I see that expo-modules-core and expo-web-browser have many Swift files in them, and I don't have any problems building or running the app without an empty Swift file of our own. The template app doesn't add one either. - Don't add a Podfile.properties.json file. Like the changes in expo/expo@dbd384b22, this would have us put certain config values in some new Expo-branded variables. We're happy keeping the values inline. - Update our value for `transformIgnorePatterns` in the Jest config, following an error when running Jest. - Looks like some sub-dependency at its updated version wants to cause different output for `generate-webview-js`. Examine that output (looks OK), and commit. Fixes: zulip#5133
chrisbobbe
force-pushed
the
pr-expo-modules
branch
from
8170e1a
to
96098a2
Compare
|
Great, thanks for the review! Revision pushed. |
Specifically, to match Expo's templates/expo-template-bare-minimum/ios/Podfile at 05f2ed0c7^ in the expo/expo repo.
Note: If you get an error with lots of lines like Entry name 'assets/fonts/AntDesign.ttf' collided , try cd android && rm -rf .gradle/ && ./gradlew clean . Done now to follow the change to templates/expo-template-bare-minimum in expo/expo@f05c8ccb6. Done by following our instructions at the top of the changed file. The second time I ran the command in those instructions, I got this helpful output: Welcome to Gradle 6.9! Here are the highlights of this release: - This is a small backport release. - Java 16 can be used to compile when used with Java toolchains - Dynamic versions can be used within plugin declarations - Native support for Apple Silicon processors For more details see https://docs.gradle.org/6.9/release-notes.html All of that looks nice and helpful.
Like we did in 196a316. Changelog: https://github.com/JetBrains/kotlin/blob/master/ChangeLog.md#1610 This seems to fix the following error we'd see on running `tools/test native --all-files` after switching from Unimodules to Expo modules (zulip#5133): ``` e: /Users/chrisbobbe/dev/zulip-mobile/node_modules/expo-sqlite/android/src/main/java/expo/modules/sqlite/SQLiteModule.kt: (106, 55): Type inference failed: Not enough information to infer parameter T in fun <reified T> arrayOfNulls(size: Int): Array<T?> Please specify it explicitly. FAILURE: Build failed with an exception. ``` Inspired to try this by expo/expo#15131 (comment) .
From `yarn why` output, I think the relevant bump might be the one from 1.21.0 to 1.21.1.
Done by mostly following the changes to Expo's templates/expo-template-bare-minimum/ in expo/expo@9781212eb. Expo's description of the new infrastructure is at https://blog.expo.dev/whats-new-in-expo-modules-infrastructure-7a7cdda81ebc . They gave a migration guide that suggested making changes similar to these, but it didn't end up helping us understand why they were the right changes to make or what would happen if we made them. That guide is at https://github.com/expo/fyi/blob/main/expo-modules-migration.md . We've cleared up some of the mystery; see zulip#5203 (comment) and previous commits in this series. In this commit: - (Mostly follow expo/expo@9781212eb, as mentioned) - Also, upgrade all our `expo-*` direct dependencies so that they work with the new system. (We upgrade them minimally, to minimize having to think about possible unrelated breaking changes right now.) Details at zulip#5203 (comment) . - Remove android/app/src/main/java/com/zulipmobile/generated/BasePackageList.java, since its only import was removed. (Expo didn't have this file in version control, and the migration guide didn't mention the file.) - The Expo commit assumes that our project has done special setup for `expo-updates`, `expo-constants`, and `expo-splash-screen`. It makes changes to that setup, which we ignore since we don't use any of those. If we need them in the future, we'll just look up their current setup instructions at the time. - Don't add an empty Swift file. The migration guide says, "A blank Swift file must be created for native modules with Swift files to work correctly." With `find node_modules | grep .swift` in our project, I see that expo-modules-core and expo-web-browser have many Swift files in them, and I don't have any problems building or running the app without an empty Swift file of our own. The template app doesn't add one either. - Don't add a Podfile.properties.json file. Like the changes in expo/expo@dbd384b22, this would have us put certain config values in some new Expo-branded variables. We're happy keeping the values inline. - Update our value for `transformIgnorePatterns` in the Jest config, following an error when running Jest. - Looks like some sub-dependency at its updated version wants to cause different output for `generate-webview-js`. Examine that output (looks OK), and commit. Fixes: zulip#5133
Note: On iOS, if the app gets to the "Connecting..." stage and
mostly hangs, not responding when you tap one of the bottom tabs,
try stopping and starting Metro a.k.a. `yarn start`.
- Ask `jest-expo` to resolve `react-test-renderer` to the right
version for `react`/`react-native`.
- Limit `react-native-reanimated` to <2.3.0. I encountered an error
like the one reported at
software-mansion/react-native-reanimated#2702
. I don't know what's causing it, but there's reportedly some
interaction with expo@43.x, which we recently added. See
software-mansion/react-native-reanimated#2702 (comment)
. We don't need 2.3.0 or later, for now. We can investigate more
later, when we do need it. At that time, we may be on a later
version of `expo`.
- For a simple way to make sure we're not affected by CVE-2022-0235,
we wanted to avoid bringing in node-fetch at one of the affected
versions. According to
GHSA-r683-j2x4-v87g
those are
>= 3.0.0, < 3.1.1
< 2.6.7
Unfortunately, there's still one chain, through `expo`, that's
bringing in node-fetch@1.7.3; see
expo/expo#16023 (comment)
- Looks like some sub-dependency at its updated version wants to
cause different output for `generate-webview-js`. Examine that
output (looks OK), and commit.
gnprice
force-pushed
the
pr-expo-modules
branch
from
96098a2
to
db7a18f
Compare
|
Looks good; merging. Thanks again for taking care of this. |
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Sep 26, 2022
Done to follow the template-app change in facebook/react-native@13107fa3d. (Which it looks like RN actually intended to make in facebook/react-native@85249cafe but forgot; that's not important, I think.) We didn't take facebook/react-native@13107fa3d on the path to the RN v0.67 upgrade because of a blocker that was resolved with the Expo 44 upgrade in expo/expo@3edc37ae4. For details, expand the `expo-screen-orientation` block in zulip#5203 (comment) and see the comment on the `Installing ExpoModulesCore 0.6.4` output from `pod install`. Done by following the "How to upgrade Gradle" instructions at the top of gradle-wrapper.properties. The result matched the template-app commit except for upstream's line endings got messed with because they didn't have Git set up properly to keep them consistent: zulip#5426 (comment) .
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Nov 22, 2022
Done to follow the template-app change in facebook/react-native@13107fa3d. (Which it looks like RN actually intended to make in facebook/react-native@85249cafe but forgot; that's not important, I think.) We didn't take facebook/react-native@13107fa3d on the path to the RN v0.67 upgrade because of a blocker that was resolved with the Expo 44 upgrade in expo/expo@3edc37ae4. For details, expand the `expo-screen-orientation` block in zulip#5203 (comment) and see the comment on the `Installing ExpoModulesCore 0.6.4` output from `pod install`. Done by following the "How to upgrade Gradle" instructions at the top of gradle-wrapper.properties. The result matched the template-app commit except for upstream's line endings got messed with because they didn't have Git set up properly to keep them consistent: zulip#5426 (comment) .
gnprice
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Nov 23, 2022
Done to follow the template-app change in facebook/react-native@13107fa3d. (Which it looks like RN actually intended to make in facebook/react-native@85249cafe but forgot; that's not important, I think.) We didn't take facebook/react-native@13107fa3d on the path to the RN v0.67 upgrade because of a blocker that was resolved with the Expo 44 upgrade in expo/expo@3edc37ae4. For details, expand the `expo-screen-orientation` block in zulip#5203 (comment) and see the comment on the `Installing ExpoModulesCore 0.6.4` output from `pod install`. Done by following the "How to upgrade Gradle" instructions at the top of gradle-wrapper.properties. The result matched the template-app commit except for upstream's line endings got messed with because they didn't have Git set up properly to keep them consistent: zulip#5426 (comment) .
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following the plan I sketched out at #5133 (comment).
I kicked the tires on Android and iOS, and everything seemed fine:
expo-apple-authentication: iOS native Apple auth workedexpo-screen-orientation: nothing odd on rotating the screenexpo-web-browser: opening a link in the "in-app browser" on iOS worked fine (and we don't use this on Android)expo-application: The app version on the Diagnostics screen showed up fineFixes: #5133