build: Upgrade to React Native 0.73 (Android) #58551
Conversation
geriux
added
[Type] Build Tooling
|
|
||
| implementation "com.github.wordpress-mobile:react-native-video:${extractPackageVersion(packageJson, 'react-native-video', 'dependencies')}" | ||
| implementation "com.github.wordpress-mobile:react-native-slider:${extractPackageVersion(packageJson, '@react-native-community/slider', 'dependencies')}" | ||
|
|
||
| // Published by `wordpress-mobile/react-native-libraries-publisher` | ||
| // See the documentation for this value in `build.gradle.kts` of `wordpress-mobile/react-native-libraries-publisher` | ||
| def reactNativeLibrariesPublisherVersion = "v4" | ||
| def reactNativeLibrariesPublisherVersion = "v5" |
There was a problem hiding this comment.
v5 will be released in this PR.
| @@ -34,7 +34,6 @@ | |||
| "@react-native-community/blur": "4.2.0", | |||
| "@react-native-community/slider": "https://raw.githubusercontent.com/wordpress-mobile/react-native-slider/v3.0.2-wp-4/react-native-community-slider-3.0.2-wp-4.tgz", | |||
| "@react-native-masked-view/masked-view": "0.3.0", | |||
| "@react-native/gradle-plugin": "0.72.11", | |||
There was a problem hiding this comment.
This is removed since React Native's has it as a dependency which supports the current AGP version.
|
Size Change: 0 B Total Size: 1.71 MB ℹ️ View Unchanged
|
| @@ -3,13 +3,100 @@ | |||
| */ | |||
| const path = require( 'path' ); | |||
|
|
|||
| // Currently all native libraries are linked manually for both platforms. | |||
There was a problem hiding this comment.
These changes are needed because we finally added this code.
Where applyNativeModulesAppBuildGradle is called, this is needed to add extra configuration like references to PackageList. Since that code also handles auto-linking, we need to reference the libraries we are linking manually, currently all of them.
There was a problem hiding this comment.
Will you expand upon why this configuration is required? Was it to address a specific error?
Is there anything blocking us from utilized auto-linking?
Tangentially related: I'm not entirely sure we need the existing configuration in this file. I believe the project option is unnecessary as we use a standard ios directory location. I believe the pre-existing dependencies configuration may actually be Metro bundler configuration. I'll research those two separately.
There was a problem hiding this comment.
Will you expand upon why this configuration is required? Was it to address a specific error?
Of course! It was to address an error when adding the following import:
apply from: file("../../../../node_modules/@react-native-community/cli-platform-android/native_modules.gradle"); applyNativeModulesAppBuildGradle(project)
It hasn't been added in the last few upgrades I guess because of errors appearing due to the auto-linking feature. I remember I saw these errors once when I tried to include this. To align our project with the current React Native files structure, this import was needed as it includes the current PackageList class definition. Used in the latest upgrades as well but we couldn't adopt it due to not applying this configuration.
As you can see now in the Kotlin file, it uses this whereas before we were using the List<ReactPackage> approach.
When applyNativeModulesAppBuildGradle is executed it goes through the project's React Native dependencies to auto-link it, but Gutenberg Mobile doesn't use the auto-linking functionality and uses the manual method instead. For Android, using the built binaries from the React Native Libraries publisher repo (demo app, bridge) and manually adding the packages.
Since I saw native_modules.gradle also contains configuration for the new architecture I think it's best to adapt it with this upgrade.
The React Native configuration changes overall are just to disable auto-linking for both platforms (we do it manually for iOS as well).
I believe the project option is unnecessary as we use a standard ios directory location. I believe the pre-existing dependencies configuration may actually be Metro bundler configuration. I'll research those two separately.
I don't think I fully understand this part, which project option are you mentioning?
There was a problem hiding this comment.
Thank you for elaborating, it helped me understand the context.
The React Native configuration changes overall are just to disable auto-linking for both platforms (we do it manually for iOS as well).
Would you be willing to link to the relevant code for iOS?
Is there anything blocking us from utilized auto-linking?
I suppose the answer my above question is that the project's desire/need to avoid a Node.js dependency in the host apps is the blocker for us utilizing auto-linking. I.e., downloading manually configured dependencies from react-native-libraries-publisher is the chosen alternative. Is that accurate?
| // Currently all native libraries are linked manually for both platforms. | |
| /** | |
| * The `null` value for each `platform` below disables auto-linking, as | |
| * we manually link these library binaries to avoid a Node.js dependency | |
| * in the host WordPress app. | |
| */ |
I suggest expanding the inline comment to clarify which code/lines it references and provide the reasoning. WDYT? Note: I wrote the above suggestion in the GitHub comment box, so there may be incorrect spacing or line lengths that could cause lint errors.
I don't think I fully understand this part, which project option are you mentioning?
I was referencing the project configuration, which may not be necessary as we use the standard ./ios location for React Native projects.
An aside: Links to PR diffs fail to reveal the highlighted portion if the diff is collapsed from being marked a "Viewed," which I have done. The URL format is also cryptic, providing little discernible information as to the targeted subject (https://github.com/WordPress/gutenberg/pull/58551/files#diff-7eaacb31e4eb9acc12388550403d16bdd0e7bfa60ba442adc0c14ed9abff4c38L325). So, I did my best to piece together the reference links from context. I generally avoid linking to portions of a GitHub PR diff because of this shortcoming in its implementation and instead link to the files themselves or individual commits. It's an annoying limitation.
There was a problem hiding this comment.
Would you be willing to link to the relevant code for iOS?
After checking this, we do have auto-linking for the demo app, I got confused with the configuration we have for Gutenberg Mobile and the XCFramework 😅 . I'll remove the ios attributes so the auto-linking keeps working for the demo app.
I suppose the answer my above question is that the project's desire/need to avoid a Node.js dependency in the host apps is the blocker for us utilizing auto-linking. I.e., downloading manually configured dependencies from react-native-libraries-publisher is the chosen alternative. Is that accurate?
I'm not sure if this is the specific case but I do know in the past the host apps were building a lot of things of Gutenberg Mobile from source like pods and maybe node_modules so it was slowing down other teams that weren't specifically working on Gutenberg.
Maybe we could investigate to enable auto-linking for the demo app but ideally it'd be good to use the same binaries the React Native Libraries publisher has for both cases.
I suggest expanding the inline comment to clarify which code/lines it references and provide the reasoning. WDYT? Note: I wrote the above suggestion in the GitHub comment box, so there may be incorrect spacing or line lengths that could cause lint errors.
👍
I was referencing the project configuration, which may not be necessary as we use the standard ./ios location for React Native projects.
That was introduced in #54077 maybe it'd be worth revisiting if it is still needed or not
There was a problem hiding this comment.
Maybe we could investigate to enable auto-linking for the demo app but ideally it'd be good to use the same binaries the React Native Libraries publisher has for both cases.
Yeah, it makes sense to rely upon a consistent approach.
That was introduced in #54077 maybe it'd be worth revisiting if it is still needed or not
Ah, OK. Thank you for sharing.
…ative package with support for AGP 8.2.1
…ually linking. This is needed to be able to use React Native's applyNativeModulesAppBuildGradle which includes needed configuration for other integration things. This auto links modules so we need to skip the ones we manually link.
geriux
force-pushed
the
build/upgrade-to-react-native-0.73-Android
branch
from
dbadd3a
to
d1387fa
Compare
…to-react-native-0.73-Android
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Looks good! Thank you for completing this. 🎉
I left a comment to help me better understand the RN CLI configuration changes, but I do not believe it should block merging this.
I will note that I have not yet tested this on a device, I figured we would begin that after all changes are within a singular branch.
Lastly, I recommend not squashing and merging this into the feature ongoing feature branch. The commit history could prove useful up until our merge into trunk. I'll leave it for you to decide, though.
| @@ -3,13 +3,100 @@ | |||
| */ | |||
| const path = require( 'path' ); | |||
|
|
|||
| // Currently all native libraries are linked manually for both platforms. | |||
There was a problem hiding this comment.
Will you expand upon why this configuration is required? Was it to address a specific error?
Is there anything blocking us from utilized auto-linking?
Tangentially related: I'm not entirely sure we need the existing configuration in this file. I believe the project option is unnecessary as we use a standard ios directory location. I believe the pre-existing dependencies configuration may actually be Metro bundler configuration. I'll research those two separately.
Good idea, how do usually do it? I only see two options available, "Squash and merge" and "Rebase and merge" I'm guessing the latter, is that correct? Thanks for the review! |
Correct, the "Rebase and merge" option would preserve the commit history. 👍🏻 |
…d in the react-native.config.js file
|
Flaky tests detected in ad4b747. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7890148207
|
|
All CI checks in Gutenberg Mobile passed including the E2E tests so I'll proceed to merge this 🚀 |
geriux
merged commit 59ae088
into
build/upgrade-to-react-native-0.73
Part of #58475
What?
This PR adds the required Android changes for the React Native
0.73.3upgrade.Why?
To keep up to date with the latest stable releases of React Native.
How?
348.1.11.8.01725.1.8937393v5Testing Instructions
CI checks related to Android should succeed.