Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump react-native to version 0.71.8 #23

Merged
merged 4 commits into from
Jun 13, 2023
Merged

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Jun 6, 2023

Bumps the React Native version to 0.71.8. This also implied updating the module substitution block to reflect the new process for fetching the React Native and Hermes modules.

How to test

  1. Run the command npm install.
  2. Run the command ./gradlew publishToMavenLocal -exclude-task prepareToPublishToS3.
  3. Observe that all libraries are built and published successfully in the local folder ~/.m2/repository/org/wordpress-mobile/react-native-libraries.

@fluiddot fluiddot self-assigned this Jun 6, 2023
@fluiddot fluiddot changed the title Bump react native to version 0.71.8 Bump react-native to version 0.71.8 Jun 6, 2023
build.gradle.kts Outdated
Comment on lines 71 to 78
substitute(module("com.facebook.react:react-android"))
.with(module("com.facebook.react:react-android:$reactNativeVersion"))
substitute(module("com.facebook.react:hermes-android"))
.with(module("com.facebook.react:hermes-android:$reactNativeVersion"))
// For backward-compatibility, we also substitute `react-native` module
// with the new module `react-android`.
substitute(module("com.facebook.react:react-native"))
.with(module("com.facebook.react:react-android:$reactNativeVersion"))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This follows the process performed by the React Native Gradle plugin (reference, docs). Additionally, as shared in this proposal, now the React Native and Hermes modules are published to Maven, similar to what we do in “react-native-mirror-publisher” repository.

Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fluiddot If the changes in this PR work as expected, they look good to me. I did leave a suggestion about removing the reactNativeMinorVersion property and the if check it's used in as I don't think it's necessary.

I also left a suggestion about the react-native-mirror to react-android change.

Neither of these are blocking this PR, so feel free to merge it as is.

build.gradle.kts Outdated Show resolved Hide resolved
build.gradle.kts Outdated
// This substitution is based on React Native Gradle plugin.
// Reference: https://t.ly/38jk
substitute(module("com.facebook.react:react-android"))
.with(module("com.facebook.react:react-android:$reactNativeVersion"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think due to this react-native to react-android change, we need to stop using our own mirror from react-native-mirror-publisher. I think we can start using the artifacts published to mavenCentral and archive the react-native-mirror-publisher project.

If we keep using our own mirror, we might get duplicate class errors. Even if we don't, it wouldn't be great to have dependencies on both react-android & react-native-mirror.

The same goes for the hermes-android, of course.

@fluiddot What do you think?

P.S: If you all decide to go through with this ^ suggestion, please let me know if you need any help!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think due to this react-native to react-android change, we need to stop using our own mirror from react-native-mirror-publisher. I think we can start using the artifacts published to mavenCentral and archive the react-native-mirror-publisher project.

That's correct. From now on, we won't need the mirror repository to publish React Native and Hermes. Hence, we could archive react-native-mirror-publisher project. In any case, I'd wait until the RN upgrade is fully released to do so.

If we keep using our own mirror, we might get duplicate class errors. Even if we don't, it wouldn't be great to have dependencies on both react-android & react-native-mirror.

The same goes for the hermes-android, of course.

@fluiddot What do you think?

P.S: If you all decide to go through with this ^ suggestion, please let me know if you need any help!

I completely agree. In fact, as part of the RN upgrade effort, I'm removing all references to the mirror in Gutenberg and WP-Android, in favor of using the ones published in mavenCentral.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great to hear!

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for putting this together. I really appreciate the comments explaining code logic, as I am unfamiliar with this repository or its scripts.

I wanted to share that I ran into the following error when following the testing instructions. I presume it is unrelated to these changes. WDYT?

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring project ':react-native-clipboard'.
> SoftwareComponentInternal with name 'release' not found.

@dcalhoun
Copy link
Member

dcalhoun commented Jun 12, 2023

I wanted to share that I ran into the following error when following the testing instructions. I presume it is unrelated to these changes. WDYT?

FAILURE: Build failed with an exception.

* What went wrong:
A problem occurred configuring project ':react-native-clipboard'.
> SoftwareComponentInternal with name 'release' not found.

I realized this error occurred as I did not run npm install within the repository before executing the testing instructions. The build succeeded this time. 🎉

I will open a separate PR updating the project README documenting setup steps.

@fluiddot
Copy link
Contributor Author

I release this error occurred as I did not run npm install within the repository before executing the testing instructions. The build succeeded this time. 🎉

Oh, sorry for that, I forgot to add that step. I'll update the PR's description.

I will open a separate PR updating the project README documenting setup steps.

Yeah, good idea. I thought about that when working on this PR, as we don't have instructions for testing 👍 .

@fluiddot
Copy link
Contributor Author

fluiddot commented Jun 12, 2023

I realized this error occurred as I did not run npm install within the repository before executing the testing instructions. The build succeeded this time. 🎉

@dcalhoun based on this comment, I assume you were able to test the PR, would you mind if I merge the PR without your explicit PR approval? Thanks!

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcalhoun based on this comment, I assume you were able to test the PR, would you mind if I merge the PR without your explicit PR approval? Thanks!

Yes, I did test and you are welcome to merge. I only abstained from an approval due to the error and my own lack of expertise in this repository. In the future, feel free to always merge as you see fit if you have approval from someone else.

@fluiddot fluiddot merged commit 3afecbd into trunk Jun 13, 2023
@fluiddot fluiddot deleted the bump-react-native-0.71.8 branch June 13, 2023 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants