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 setup-nightly script for 3rd party libs to test nightly builds #34838

Closed
wants to merge 1 commit into from

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Oct 2, 2022

Summary

Most third-party libraries used to deal with react-native breaking changes by checking the package version, e.g. expo or reanimated. However, the nightly build version on npm is something like 0.0.0-20221001-2020-436362670. If we check the version, that would be the oldest version.

I've tried to visit some proper solutions. If we try to publish nightly versions as 0.999.0 or even 1000.999.0. That would be strange because there're newer published versions on npm than release versions. Maybe the current state to keep the 0.0.0 as nightly builds would still be a good idea. As a workaround, this PR tries to add the setup-nightly.js postinstall script. It will rewrite the version after installing the package:

  • Rewrite version in package.json as 999.999.999
  • Rewrite VERSION_NAME in ReactAndroid/gradle.properties as 999.999.999

Changelog

[General] [Fixed] - Fix nightly builds break version detection for third-party libraries

Test Plan

Test install local pakcage

  1. cd /path/to/react-native ; npm pack --pack-destination $HOME
  2. cd /path/to/app ; yarn add file:$HOME/react-native-1000.0.0.tgz
    check whether the versions in pacakge.json and gradle.properties are 999.999.999
  3. test build and launch both on android and ios

Publish nightly build

I didn't test the publishing workflow. Maybe we should check whether CI results.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Expo Partner: Expo Partner labels Oct 2, 2022
@github-actions
Copy link

github-actions bot commented Oct 2, 2022

Warnings
⚠️ 🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against c4349b1

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,736,984 +0
android hermes armeabi-v7a 7,141,805 +0
android hermes x86 8,046,372 +0
android hermes x86_64 8,018,597 +0
android jsc arm64-v8a 9,605,087 +0
android jsc armeabi-v7a 8,372,677 +0
android jsc x86 9,551,020 +0
android jsc x86_64 10,144,034 +0

Base commit: 4363626
Branch: main

@cortinico
Copy link
Contributor

Can you elaborate on what are the 3rd party lib assumptions you refer to here?

// Version for nightly build. Since third party libraries used to check the minor version for feature detections.
// The nightly build version we published to npm is `0.0.0` which will break third party libraries assumption

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 403fea2
Branch: main

@Kudo
Copy link
Contributor Author

Kudo commented Oct 2, 2022

Can you elaborate on what are the 3rd party lib assumptions you refer to here?

for instance,

if you need more examples, please let me know. i'll try to find more for you. thanks!

@Kudo Kudo marked this pull request as ready for review October 2, 2022 15:09
@Kudo Kudo requested a review from hramos as a code owner October 2, 2022 15:09
@cortinico
Copy link
Contributor

cortinico commented Oct 2, 2022

if you need more examples, please let me know. i'll try to find more for you. thanks!

It seems that both Expo and Reanimated rely on the VERSION_NAME inside ReactAndroid/gradle.propreties:
https://cs.github.com/expo/expo/blob/f2b83416fd7bdf6cee98c7815cf29c32fa5577eb/packages/expo-modules-core/android/build.gradle#L76

IMHO this should not be fixed by a version change inside React Native but by a version parsing change on your end. You can easily do something like:

def REACT_NATIVE_IS_NIGHTLY = reactProperties.getProperty("VERSION_NAME").startsWith("0.0.0")

You can then pass over this information to your Gradle/CMake config and use it accordingly. Or you can override your REACT_NATIVE_TARGET_VERSION to the upcoming stable (71) if IS_NIGHTLY is true.

I would refrain from:

  1. Having a post-install script as that could expose us to security implications. Especially in this case the post-install script is only on nightlies and hard to spot as we're effectively manipulating the package.json which is something we should look into moving away from.
  2. Tweaking the version number as that might have unexpected side effects. Eg. if a user tries to ./gradlew :ReactAndroid:publishToMavenLocal, they will be publishing 999. to their Maven Local. This version might will affect their local build for future RN projects (as it's higher than 0.x).

EDIT: Typo

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Oct 2, 2022
@Kudo
Copy link
Contributor Author

Kudo commented Oct 3, 2022

IMHO this should not be fixed by a version change inside React Native but by a version parsing change on your end. You can easily do something like:

def REACT_NATIVE_IS_NIGHTLY = reactProperties.getProperty("VERSION_NAME").startsWith("0.0.0")

this is fair enough to me. however, it needs third party libraries to update their build settings. i will start this change from expo and share how to integrate nightly testing to the community.
i'm closing the pr. thanks @cortinico!

@Kudo Kudo closed this Oct 3, 2022
@Kudo Kudo deleted the setup-nightly branch October 3, 2022 01:16
@cortinico
Copy link
Contributor

this is fair enough to me. however, it needs third party libraries to update their build settings. i will start this change from expo and share how to integrate nightly testing to the community.

I understand. Still third party libraries should not need to know the RN minor version. I can understand that Expo and Reanimated are edge case in this sense and might need specific APIs.

Still, I'm happy to discuss how we can expose APIs from the framework in a more structured way instead of relying on string comparisons on the version number 👍

@Kudo
Copy link
Contributor Author

Kudo commented Oct 3, 2022

Still, I'm happy to discuss how we can expose APIs from the framework in a more structured way instead of relying on string comparisons on the version number 👍

thanks for the kindness. i would say the version detection so far is the reliable way. there are various types of breaking changes, i don't think we have to pay effort on exposing stable API in the meantime.

let's just keep it as-is.
there are still other issues from nightly build. i will create another pr to address them.

@Kudo
Copy link
Contributor Author

Kudo commented Oct 3, 2022

there are still other issues from nightly build. i will create another pr to address them.

the pr is at #34846

Kudo added a commit to expo/expo that referenced this pull request Nov 1, 2022
# Why

follow up with facebook/react-native#34838 (comment) to support nightly build

# How

override react-native version as `9999.9999.9999` when it's `0.0.0-` prefixed

# Test Plan

1. adding a react-native nightly version and remove all 3rd party libraries from bare-expo
2. do some manual changes
  - _packages/expo/android/src/main/java/expo/modules/ReactNativeHostWrapperBase.kt_ to remove deprecated `getUIImplementationProvider`
3. test ios / android build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants