-
Notifications
You must be signed in to change notification settings - Fork 127
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
base: main
Are you sure you want to change the base?
Conversation
f3161dc
to
6e8e885
Compare
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.
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?
Do you expect frameworks to no longer need their configuration files and use this config instead, or will frameworks still need their own configuration? |
Hi @TheSavior, thanks for the questions:
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:
The manifest will be versioned.
In a future version, let's say K, we will set the default value to 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.
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 Then, if a framework offers some custom feature that need to be configured, they can use their own configuration file. Does this make sense? |
Can we include, or at least mention, out-of-tree platforms in the proposal? They're missed out imho |
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. |
Hi there, @TheSavior @kelset @thymikee! |
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 For Android, instead, they have a completely different mechanism with the The idea of this proposal is exactly the one of removing this differences and this implicit knowledge: by operating on a simpler |
There was a problem hiding this 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.
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YES PLEASE
reactNativeManifest
to package.json
for React Native specific metadata (Take 3)
reactNativeManifest
to package.json
for React Native specific metadata (Take 3)reactNativeManifest
to package.json
for React Native specific metadata (Take 3)
189c660
to
0aefd3d
Compare
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 😊
@cipolleschi I just think my ideal state of things is closer to having a one well-defined config per platform (like All that saying, I don't want to block this effort and to me it's a step in a good direction 👍🏼 |
@thymikee I understand, but if we go for platform specific higher level setups we are not unifying the platforms. If we ask our users to modify
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 WDYT? |
Yeah I'm good with that |
{ | ||
"reactNativeManifest": { | ||
"capabilities": { | ||
"codegenConfig": { |
There was a problem hiding this comment.
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
?
"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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 |
(just leaving this here, might be good to know/relevant for package.json: https://socket.dev/blog/openjs-improve-interoperability-of-javascript-package-metadata ) |
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:
This will not be the place where we can add any kind of configuration.
Practically, this RFC proposes to add a
reactNativeManifest
section to thepackage.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: