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 a field for newArchEnabled and hermesEnabled in react-native info #1809

Closed
cortinico opened this issue Jan 25, 2023 · 9 comments · Fixed by #1897
Closed

Add a field for newArchEnabled and hermesEnabled in react-native info #1809

cortinico opened this issue Jan 25, 2023 · 9 comments · Fixed by #1897

Comments

@cortinico
Copy link
Member

Describe the Feature

As the title says, it would be great to add a field for newArchEnabled and hermesEnabled in react-native info. We receive several issue reports on facebook/react-native and most of the time we need to ask if Hermes/NewArch has been enabled or disabled. Would be great to have it from the info command.

Possible Implementations

Ideally we should have it split between Android & iOS
Android: read from the android/gradle.propreties file
iOS: read from the ios/Podfile file.

@thymikee
Copy link
Member

Good idea, let's do it

@adamTrz
Copy link
Collaborator

adamTrz commented Feb 2, 2023

Hi @cortinico
While getting those fields from android/gradle.propreties shouldn't be an issue, I'm having an issue figuring out iOS part..
We're setting newArch enabled for iOS by running RCT_NEW_ARCH_ENABLED=1 pod install right? How can we detect this? 🤔

@cipolleschi
Copy link
Contributor

Hi @adamTrz, trying to answer for the iOS related part.

Currently, there isn't an easy way to do so, unfortunately, but we can easily build it if needed.
The way to do it right now is:

  • For the newArchEnabled equivalent, when we install the pods, you can look whether the Pods/Pods.xcodeproj file contains the RCT_NEW_ARCH_ENABLED flag and it is set to 1. Currently, if there is no flag, it means that we are on the old architecture.
  • For hermesEnabled, you can read the Podfile.lock and check whether hermes_engine is there or not. If it's there, it means that the app is running hermes, if not, it is running JSC.

I understand that it is not ideal. We can update the Cocoapods scripts to write those information somewhere that it is confortable for you to read.
It could be an xcconfig file or we can see how to modify the Info.plist to add those information.

What do you think would be the best?

@adamTrz
Copy link
Collaborator

adamTrz commented Feb 2, 2023

Thanks for reply @cipolleschi :)
I think this is doable, but both points are starting with the assumption that user already did pod install. So react-native info won't be beneficial for those who encountered some errors doing pods installation...

Maybe a solution would be to leverage reactNativeMetadata field from package.json (as discussed here) and read configuration from there?
We could do similar stuff to what Expo is doing where they read config from Podfile.properties.json and apply stuff into Podfile from it? Or just bluntly copy their solution and create same file inside /ios dir? 😁

WDYT?

Edit: Also noticed that currently by default we're reading flags from flags that are returned from get_default_flags ruby function so we have to keep this in mind as well? 🤔

@cipolleschi
Copy link
Contributor

cipolleschi commented Feb 2, 2023

I understand the use case.

I have two concerns if we just add a configuration file (of any kind) in the current situation:

  1. the information in the config file could not be the true one. The Podfile will always be in the user space, hence the user can modify it. That means that, even if the config says, for example, newArch=true; useHermes=true if the user changed the Podfile manually or run the pod install command with some flag, the information returned by react-native info won't be accurate.
  2. It won't be backward compatible. Say that we introduce the change in 0.72, users of 0.71 that run react-native info won't have this information.

I think we can live with 2, honestly, as we can't really make all the changes backward compatible and, as soon as we release 0.75, all the supported versions of React Native would have that information.

For 1. we need to do some more work to make it reliable. Specifically, we should also hide the new_arch_enabled and hermes_enabled parameters from the ruby script, disable the RCT_NEW_ARCH_ENABLED and USE_HERMES flags and rely on a single source of truth (which should be the same for iOS and Android).

Honestly, I really like this approach. It would streamline the experience of our users and I think that the package.json's reactNativeMetadata section would be perfect for this.

@cortinico what do you think about this?

@adamTrz
Copy link
Collaborator

adamTrz commented Feb 2, 2023

Super into it, big fan of one source of truth. Granted, it would require some work to be done but then users would have an option to opt-in or out of extra features in one place and not to jump between files or set some env variables.

@cortinico
Copy link
Member Author

Maybe a solution would be to leverage reactNativeMetadata field from package.json (as discussed react-native-community/discussions-and-proposals#588) and read configuration from there?

My 2 cents here. Yes this would definitely help and simplify the implementation. If RFC588 goes through, our tooling will have to be adapted to account for that.

On the other hand, this feature for react-native info shouldn't be blocked by that RFC. The question here is if we're able to know if the user has executed RCT_NEW_ARCH_ENABLED=1 pod install by looking at the codebase (i.e. checking in build/generated folders for ios is also a valid solution). That's possible on Android, not sure if this is possible on ios reliably.

I would not mix the implementation details of reactNativeMetadata here in this issue. Once that RFC lands, we'll update react-native info to query reactNativeMetadata first, and fallback to the previous logic.

@cipolleschi
Copy link
Contributor

@cortinico I think that the main concern here is how to retrieve those information in case pod install failed.

I think that the algorithm should go in this way:

  1. check whether the Podfile.lock exists
  2. if it exists:
    1. if hermes-engine is present => set hermesEnabled to true
    2. read the *.xcodeproject file. If it contains the -DRCT_NEW_ARCH_ENABLED=1 flag, set newArchEnabled to true
  3. if it doesn't exists, assume defaults:
    1. hermesEnabled to true
    2. newArchEnabled to false

@cortinico
Copy link
Member Author

Yup your algo makes sense to me @cipolleschi 👍

if it doesn't exists, assume defaults:

I would rather report "value not found" or something similar rather than a default. The info command has support for this as well IIRC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants