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

RFC0717: Introducing reactNativeManifest to package.json for React Native specific metadata (Take 3) #717

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

cipolleschi
Copy link

@cipolleschi cipolleschi commented Oct 6, 2023

This RFC introduces and formally discuss the concept of React Native Manifest, a set of metadata about a React Native project (either app or library). This metadata can be consumed by tooling to enhance the developer experience and support the React Native ecosystem.

This would be the entry point for all the official configurations supported by the core of React Native. Examples (not an exhaustive list) of these configurations could be:

  • Whether the New Architecture is enabled or not.
  • Which JS Engine should run in the app.
    This will not be the place where we can add any kind of configuration.

Practically, this RFC proposes to add a reactNativeManifest section to the package.json file of a React Native app or library.
This new section will allow developers to follow a declarative approach to express capabilities of apps and libraries in a more formalized manner.

Rendered Version of this RFC

History:

@cipolleschi cipolleschi marked this pull request as draft October 6, 2023 09:54
@elicwhite
Copy link

elicwhite commented Oct 6, 2023

as of today, there is no simple programmatic way to know if a NPM package is a React Native library

I may have missed this in a previous discussion, but what is the goal of being able to detect React Native libraries? An issue is that plenty of npm packages are JS only and work great in React Native. So if the goal is to have it be a filter for the libraries we propose to people, that would miss many compatible libraries.

Lack of a declarative way to define if a library is compatible with the New Architecture

For flagging whether the new arch is enabled or not, we will eventually make the new arch the default. At that point would the flag in the package.json be required forever? If not, if a package didn't have the flag anymore does that mean that the package is old and unmaintained, or the package is new and supports the new architecture?

This need is so evident that various frameworks provided their own ways to specify those values. Look at react-native.config.js for the CLI or app.json for Expo.

Do you expect frameworks to no longer need their configuration files and use this config instead, or will frameworks still need their own configuration?

@cipolleschi
Copy link
Author

Hi @TheSavior, thanks for the questions:

what is the goal of being able to detect React Native libraries? An issue is that plenty of npm packages are JS only and work great in React Native. So if the goal is to have it be a filter for the libraries we propose to people, that would miss many compatible libraries.

In the context of this RFC, the React Native libraries are meant as libraries that requires React Native to work. Most of plain JS libraries that works on Web will also work on React Native, but we may need a way to isolate the libraries that requires React Native to work for various reasons. Top of mind:

  • run statistics on how many libs for React Native are out there to check how healthy our ecosystem is
  • to monitor the migration to the New Architecture (or any other migration we might need to monitor in the future)

For flagging whether the new arch is enabled or not, we will eventually make the new arch the default. At that point would the flag in the package.json be required forever? If not, if a package didn't have the flag anymore does that mean that the package is old and unmaintained, or the package is new and supports the new architecture?

The manifest will be versioned.
Version 1.0.0 has the semantic:

  • newArchitecture default value set to false

In a future version, let's say K, we will set the default value to true and deprecate the fields (when the New Architecture is enabled by default)

In a more future version, let's say N, we will remove the field completely, as we would have remove the Old Architecture from React Native.

Unmaintained/old packages would not have the manifest or an old version of the manifest with its own semantic.

Do you expect frameworks to no longer need their configuration files and use this config instead, or will frameworks still need their own configuration?

They will stil need those, but for their own configurations. What we are trying to do here is to provide the source of truth for the configurations of the core of React Native. We don't want that a framework redefine how the New Architecture is enabled, for example. If they need that information, they will have to read it from the package.json of the app/library.

Then, if a framework offers some custom feature that need to be configured, they can use their own configuration file.

Does this make sense?

@cipolleschi cipolleschi marked this pull request as ready for review October 9, 2023 13:47
@thymikee
Copy link
Member

thymikee commented Oct 9, 2023

Can we include, or at least mention, out-of-tree platforms in the proposal? They're missed out imho

@kelset
Copy link
Member

kelset commented Oct 10, 2023

Thanks @cipolleschi for the write up! I very much like the clarity and level of details that you've put into the RFC. I left a few comments but they are mostly nits, overall I like it a lot and I think that we might finally be seeing the light at the end of this tunnel!

One more thought: I agree with @thymikee, we need to mention clearly how other platforms can leverage this manifest. I don't think there's anything major that needs to be added to the RFC since the rules being set for "platform over global config" are generic already, so I assume just a quick section saying something like "for out of tree platform, you can just go ahead and make reactNativeManifest.platform name.capabilities and follow the schema versioning to be consistent with the one provided for ios and android" or something like it.

@cipolleschi
Copy link
Author

Hi there, @TheSavior @kelset @thymikee!
I integrated your feedback directly in the proposal. You can see the changes in the latest commit (189c660). Let me know what do you think and if we can move forward with the proposal.

@thymikee
Copy link
Member

One last thing I’d like this proposal to consider is to only serve libraries, not apps. There’s already a lot of configuration for front end projects. Adding one more for product teams is not ideal imho. Instead we could add contracts for the tooling like Expo/RN CLI to infer—such as reading from podfiles or gradle config or else that easily scales for other platforms as well

@cipolleschi
Copy link
Author

cipolleschi commented Oct 17, 2023

One last thing I’d like this proposal to consider is to only serve libraries, not apps. There’s already a lot of configuration for front end projects. Adding one more for product teams is not ideal imho. Instead we could add contracts for the tooling like Expo/RN CLI to infer—such as reading from podfiles or gradle config or else that easily scales for other platforms as well

@thymikee I don't think that this is the way to go. For app, this proposal could standardize how our general user interact with React Native, and make it much easier.

For example, for iOS, a user needs to know about various flags, for example RCT_NEW_ARCH_ENABLED and USE_HERMES, and they have to know that they need to feed those flags to the pod install command.

For Android, instead, they have a completely different mechanism with the gradle.properties file.

The idea of this proposal is exactly the one of removing this differences and this implicit knowledge: by operating on a simpler json files they can use the same mechanism to configure their iOS and Android apps.
This also remove the requirement for them to know how the native platforms works: they don't need to know about Cocoapods and gradle.

Copy link
Member

@kelset kelset left a comment

Choose a reason for hiding this comment

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

Thanks for all your work @cipolleschi! This overall looks great. I left a couple of super nits comments, overall I'll ✅ but also I'll pass it internally so that other folks can take a look and give their perspectives.

We should also make sure that someone from Expo (@brentvatne?) gives their pass to this before proceeding.

proposals/0012-introduce-reactNativeMetadata.md Outdated Show resolved Hide resolved

Most of those config flags are undocumented and they're not verified for invalid configuration. Specifically, some feature flags require to be toggled on multiple layers, and it's easy to forget to toggle one of them. Invalid combinations of those flags could lead to unexpected runtime behavior.

We propose to define `reactNativeManifest` as the **preferred entry point** for all the user facing entry points. All the tooling should be adapted to consume the information from the `reactNativeManifest` section and have sensible defaults defined by the tooling itself.
Copy link
Member

Choose a reason for hiding this comment

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

YES PLEASE

@kelset kelset changed the title ReactNativeManifest | Take 3 Introducing reactNativeManifest to package.json for React Native specific metadata (Take 3) Oct 24, 2023
@kelset kelset changed the title Introducing reactNativeManifest to package.json for React Native specific metadata (Take 3) RFC0717: Introducing reactNativeManifest to package.json for React Native specific metadata (Take 3) Oct 24, 2023
@cipolleschi cipolleschi force-pushed the cipolleschi/rfc-reactNativeManifest branch from 189c660 to 0aefd3d Compare November 6, 2023 10:21
@cipolleschi
Copy link
Author

Hello there! Reviving this discussion.

@brentvatne did you have the chance to review this?

@thymikee any thoughts about my answer here?

@tido64 (tagging you as Kelset is OOO) do you know if anyone from MSFT has some concern? Kelset said that he was going to share this internally, but I heard nothing after that.

1. If the capability `foo` is specificed in the `reactNativeManifest.capabilities` section, use the value specified in the `reactNativeManifest.capabilities.foo` section.
1. If the user is **also** specifying the capability `foo` in the build tool specific configuration (e.g. inside `gradle.properties` for Android) behave as follows:
1. If the two values are **compatible**, use them without notifying the user.
1. If the two values are **incompatible**, notify the user and use the value specified in the `reactNativeManifest.capabilities.foo` section will prevail and the value specified in the build tool specific configuration will be ignored.
Copy link
Collaborator

Choose a reason for hiding this comment

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

For some logic this is fine. For build logic, the app has to be built either including or not including the new architecture files. But for runtime logic I'm concerned about having the package.json values win. How can you write experimentation code that flights A/B testing of enabling a capability if the value from package.json wins?

Copy link
Author

@cipolleschi cipolleschi Nov 7, 2023

Choose a reason for hiding this comment

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

I'm not sure I'm following.
At runtime, you don't have access to the package.json. AFAIK, it is not shipped in the iOS bundle.
The way we expect it to work is that gradle/cocoapods reads the package.json and configures the build accordingly, eventually passing some flags to the build system.

It is actually already like that for some setitng like the NewArch enabled and hermes.
For the runtime flags, if those flags are controlled by equivalent build time flag (#ifdefs, to make it clear), you still can't change them at runtime. Otherwise, it is possible to write (and probably you already have) some code that fetch some configuration from a server and updates those flags.
In the case of A/B Tests, the package.json should specify the base configuration.

Am I missing something?

Copy link
Author

Choose a reason for hiding this comment

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

gentle nudge for @acoates-ms and @tido64. 😊

@thymikee
Copy link
Member

@cipolleschi I just think my ideal state of things is closer to having a one well-defined config per platform (like gradle.properties for Android) that is further abstracted to a JSON config by frameworks which already have their own config files, and would know how to configure the platforms. In other words: React Native (and out of tree platforms) would be responsible for standardizing the config per platform and frameworks for the DX of how it's done through a single config file.

All that saying, I don't want to block this effort and to me it's a step in a good direction 👍🏼

@cipolleschi
Copy link
Author

@thymikee I understand, but if we go for platform specific higher level setups we are not unifying the platforms.
One of the good things of React Native is that a user "don't need" to know the platforms to start developing with it.

If we ask our users to modify grandle.properties and Info.plist files, we are:

  • leaking the details of the platforms at user level.
  • asking them to do the same job twice (or more, one time for each platform).

This proposal is an attempt to avoid those problems, providing the user a familiar interface that can use to set up its project.

I agree that frameworks might have their settings and configurations: nothing stops them to use those configurations to update the package.json or to read what's in the package.json before carry on their work.

WDYT?

@thymikee
Copy link
Member

Yeah I'm good with that

{
"reactNativeManifest": {
"capabilities": {
"codegenConfig": {
Copy link

@kraenhansen kraenhansen Nov 14, 2023

Choose a reason for hiding this comment

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

Nit: Since all of the capabilities values are essentially configuration of different capabilities, I believe it make sense to simplify this name and make it just codegen?

Suggested change
"codegenConfig": {
"codegen": {


## Alternatives

`reactNativeManifest` is, by design, fully original and new: there are other files that are used to define configurations for various aspect of react-native based projects (such as `react-native.config.js`, `app.json`, Expo's `app.config.js`, `expo-module.config.json`) but this one does **not** overlap with any of them. Explicitly, this one has the purpose of being filled only with capabilities and metadata to be read by package managers and build tools.
Copy link
Contributor

Choose a reason for hiding this comment

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

In React Native template we have app.json file, and what's the role of this file in React Native CLI projects? It was added here, for npx react-native eject purpose. eject command is a historical thing. What values should we put inside app.json in CLI projects, and what in react-native.config.js file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am also curious about app.json and why it should continue to exist. It seems like a lot of the configuration there, such as the expo stuff, could be in the package.json.

@notbrent gave some more context here: react-native-community/cli#1113 (comment)

I realize that's a side-issue from this PR, though.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, this issue focus on the parameters that React Native core needs to actually work and to be configured.

Personally, I believe that the app.json file, together with the eject commands should be removed completely as they are not really a thing as of today.

and what in react-native.config.js file.

The react-native.config.js is an additional and optional file that can be added to the project. It is independent from this proposal. As far as I understand, it can be used to add some configurations that are helpful to the CLI.
For that reason, if you need the info from the app.json, it should be merged into the react-native.config.js so that we reduce the number of files required by the template.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, so app.json right now contains name which is then used inside index.js file to register root component, where should we put this value, after app.json deletion? I don't think that we should put this inside react-native.config.js, because then we need to add react-native.config.js to the template.

Copy link
Author

Choose a reason for hiding this comment

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

Can't we just hardcode it into the index.js and update the CLI to replace that string? Unless it is used for something else, I don't understand what's the pro of externalize it.

@brentvatne
Copy link
Contributor

brentvatne commented Dec 23, 2023

@brentvatne did you have the chance to review this?

Apologies for the delay! This got lost in my notifications. I'm in favor of using this config for libraries but I'm not convinced that it's needed for apps.

We had a similar discussion at Expo, and what we ended up deciding on was to copy the gradle.properties pattern from Android to iOS, so we made a Podfile.properties.json and we read from that to configure various things like the Hermes engine and the New Architecture. This makes it pretty easy for us to introduce new fields, and for other libraries in the ecosystem to do the same. For example, we defined some properties to configure gif/webp configuration and to opt in / out of network debugging.

In an Expo project that uses the New Architecture on iOS, Podfile.properties.json looks like this:

{
  "expo.jsEngine": "hermes",
  "EX_DEV_CLIENT_NETWORK_INSPECTOR": "true",
  "newArchEnabled": "true"
}

And this is how we read it in the Podfile:

podfile_properties = JSON.parse(File.read(File.join(__dir__, 'Podfile.properties.json'))) rescue {}

ENV['RCT_NEW_ARCH_ENABLED'] = podfile_properties['newArchEnabled'] == 'true' ? '1' : '0'
ENV['EX_DEV_CLIENT_NETWORK_INSPECTOR'] = podfile_properties['EX_DEV_CLIENT_NETWORK_INSPECTOR']

Our gradle.properties looks like this:

# .....other fields above this

reactNativeArchitectures=armeabi-v7a,arm64-v8a,x86,x86_64
newArchEnabled=true
hermesEnabled=true

# Enable GIF support in React Native images (~200 B increase)
expo.gif.enabled=true
# Enable webp support in React Native images (~85 KB increase)
expo.webp.enabled=true
# Enable animated webp support (~3.4 MB increase)
# Disabled by default because iOS doesn't support animated webp
expo.webp.animated=false

# Enable network inspector
EX_DEV_CLIENT_NETWORK_INSPECTOR=true

Sidenote: If a developer uses prebuild then the ios and android directories will be thrown away and re-generated often. In this case, they can use expo-build-properties config plugin to set the values in gradle.properties / Podfile.properties.json. But given that React Native doesn't have this concept, it's not really worth thinking about too much here.

@kelset
Copy link
Member

kelset commented Feb 27, 2024

(just leaving this here, might be good to know/relevant for package.json: https://socket.dev/blog/openjs-improve-interoperability-of-javascript-package-metadata )

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.

10 participants