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

Add react-native-reanimated library with version 2.17.0 #24

Merged
merged 2 commits into from
Jun 13, 2023

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Jun 6, 2023

Adds the Reanimated library. In newer versions, we no longer need the forked repository because we can now set the Hermes engine using an environment variable. Previously, we had to enforce this by updating the build configuration (reference).

NOTE: This version only works in newer versions of React Native, hence it was branched off from #23.

How to test

  1. Run the command npm install.
  2. Run the command CLIENT_SIDE_BUILD="True" JS_RUNTIME="hermes" ./gradlew react-native-reanimated:publishToMavenLocal -exclude-task prepareToPublishToS3
  3. Observe that the library is 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 Add react native reanimated with version 2.17.0 Add react.native-reanimated with version 2.17.0 Jun 6, 2023
@fluiddot fluiddot changed the title Add react.native-reanimated with version 2.17.0 Add react-native-reanimated with version 2.17.0 Jun 6, 2023
@fluiddot fluiddot changed the title Add react-native-reanimated with version 2.17.0 Add react-native-reanimated library with version 2.17.0 Jun 6, 2023
@fluiddot fluiddot force-pushed the add-react-native-reanimated-2.17.0 branch from 51bb208 to f492a13 Compare June 7, 2023 09:47
@oguzkocer
Copy link
Contributor

@fluiddot I'd be happy to review this and the react-native upgrade PR. However, I think it might be a couple days before I can get to it. I hope that's OK and the PRs are not urgent.

@fluiddot
Copy link
Contributor Author

fluiddot commented Jun 8, 2023

@fluiddot I'd be happy to review this and the react-native upgrade PR. However, I think it might be a couple days before I can get to it. I hope that's OK and the PRs are not urgent.

Sure, no problem @oguzkocer. In the meantime, I'm testing the new versions in Gutenberg Mobile using the binaries published in the Maven local folder.

@fluiddot fluiddot changed the base branch from trunk to bump-react-native-0.71.8 June 8, 2023 08:13
@fluiddot fluiddot marked this pull request as ready for review June 8, 2023 08:13
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 This looks good to me as is - and this might be more of a personal taste - but I think adding the environment variables outside gradle command for the whole script might look cleaner.

This is how I would go about it:

# react-native-reanimated library uses JSC by default. These env vars will force it to use Hermes instead.
# https://github.com/software-mansion/react-native-reanimated/blob/dea5cc2c713724ee1705daea62d18733c4ce4127/android/build.gradle#L232-L252
export CLIENT_SIDE_BUILD="True"
export JS_RUNTIME="hermes"

This ^ would be added before the for loop and I'd remove the if check and simply do ./gradlew :$project:publishS3PublicationToS3Repository as we used to do.

Having said that, this is just a suggestion and please feel free to merge it as is if it's working as intended for you.

.buildkite/publish-libraries.sh Outdated Show resolved Hide resolved
@fluiddot
Copy link
Contributor Author

@fluiddot This looks good to me as is - and this might be more of a personal taste - but I think adding the environment variables outside gradle command for the whole script might look cleaner.

This is how I would go about it:

# react-native-reanimated library uses JSC by default. These env vars will force it to use Hermes instead.
# https://github.com/software-mansion/react-native-reanimated/blob/dea5cc2c713724ee1705daea62d18733c4ce4127/android/build.gradle#L232-L252
export CLIENT_SIDE_BUILD="True"
export JS_RUNTIME="hermes"

This ^ would be added before the for loop and I'd remove the if check and simply do ./gradlew :$project:publishS3PublicationToS3Repository as we used to do.

Having said that, this is just a suggestion and please feel free to merge it as is if it's working as intended for you.

My original approach was actually using environment variables but I was hesitant to expose them for all modules, that's why I went with this approach. AFAIK, apart from Reanimated, no other module will use them when building so we could have them globally. I'm fine updating it using env vars 👍 .

@fluiddot fluiddot force-pushed the add-react-native-reanimated-2.17.0 branch from f492a13 to d00056f Compare June 12, 2023 09:36
@fluiddot
Copy link
Contributor Author

My original approach was actually using environment variables but I was hesitant to expose them for all modules, that's why I went with this approach. AFAIK, apart from Reanimated, no other module will use them when building so we could have them globally. I'm fine updating it using env vars 👍 .

I applied the suggestion in d00056f.

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.

These changes make sense to me. The testing instructions succeeded for me. Thank you for outlining the rationale for the changes. 🙇🏻

@oguzkocer
Copy link
Contributor

My original approach was actually using environment variables but I was hesitant to expose them for all modules, that's why I went with this approach. AFAIK, apart from Reanimated, no other module will use them when building so we could have them globally. I'm fine updating it using env vars 👍 .

Thanks for the update @fluiddot, it looks great!

Base automatically changed from bump-react-native-0.71.8 to trunk June 13, 2023 08:11
@fluiddot fluiddot merged commit 95a3c2e into trunk Jun 13, 2023
@fluiddot fluiddot deleted the add-react-native-reanimated-2.17.0 branch June 13, 2023 08:20
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