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

RFC: introduce reactNativeManifest to package.json #588

Closed
wants to merge 11 commits into from

Conversation

kelset
Copy link
Member

@kelset kelset commented Jan 25, 2023

This RFC wants to introduce and formally discuss a new section for the package.json of a react-native project (app or library), called reactNativeMetadata.

This new section will allow developer to express certain characteristics of their code in a more formalized manner, that can easily used by tooling to act towards the code accordingly.

Note well: this is but the first draft of the actual shape and fields. The goal of this draft is to collect feedback and help finalise the spec for this section.


You can read more details in the text itself - here's the file view for easier reading experience.

@kelset kelset added the 💡 Proposal This label identifies a proposal label Jan 25, 2023
Copy link
Member

@cortinico cortinico 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 the writeup @kelset
I'll do a further review in the next days. I'll just left one thoughts I had on top of my mind

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

## Alternatives

`reactNativeMeta` 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 metadata to be read by package managers and other tools.
Copy link

Choose a reason for hiding this comment

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

As this grows, or if we want to allow it to be dynamic, or extendable from a base, it seems beneficial to allow mirroring into a react-native.config.js outside the direct package.json (maybe not named that since already taken). E.g. see recent template change moving Jest config out of package.json (or... cosmic-config bits).

Copy link
Member Author

Choose a reason for hiding this comment

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

react-native.config.js already exists and is used for CLI stuff (see https://github.com/react-native-community/cli/blob/main/docs/configuration.md#configuration) - it is an explicit goal of this metadata field to be entirely separate.

Copy link

Choose a reason for hiding this comment

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

Call it something else as an example, but it seems like the app fields are being used for configuration. If that configuration becomes non-trivial, it may scale badly inlined into each package.json vs being shareable. Like, say, a root eslint.config.js or jest.config.js. The mapping of foo in package.json to foo.config.js is also somewhat expected by convention imo.

Choose a reason for hiding this comment

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

filled only with metadata to be read by package managers and other tools

What does this mean? It seems like it allows it to be used for almost anything.

Copy link
Member

Choose a reason for hiding this comment

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

As this grows, or if we want to allow it to be dynamic, or extendable from a base, it seems beneficial to allow mirroring into a react-native.config.js outside the direct package.json

Could you clarify on why we want this now? This is something we can still add later if we wish and the current design is not preventing it. Or am I missing something?

proposals/0012-introduce-reactNativeMeta.md Outdated Show resolved Hide resolved
proposals/0012-introduce-reactNativeMeta.md Outdated Show resolved Hide resolved
@kelset kelset changed the title RFC: introduce reactNativeMeta to package.json RFC: introduce reactNativeMetadata to package.json Jan 27, 2023
proposals/0012-introduce-reactNativeMetadata.md Outdated Show resolved Hide resolved
proposals/0012-introduce-reactNativeMetadata.md Outdated Show resolved Hide resolved
proposals/0012-introduce-reactNativeMetadata.md Outdated Show resolved Hide resolved
proposals/0012-introduce-reactNativeMetadata.md Outdated Show resolved Hide resolved
proposals/0012-introduce-reactNativeMetadata.md Outdated Show resolved Hide resolved
@motiz88
Copy link
Collaborator

motiz88 commented Feb 13, 2023

package.json of a react-native project (app or library), called reactNativeMetadata

High-level comment: I don't think solving for apps and libraries with the same mechanism is the right call. package.json should ideally contain only configuration/metadata that applies to the code within that package, makes sense in the published version of the package, and is respected by all relevant tools when the package is transitively included in an app project. App-level config should live elsewhere (and probably already does).

@kelset
Copy link
Member Author

kelset commented Feb 16, 2023

hey folks - quick update here: there have been some conversations happening behind the scenes but current goal is to get to a new level of alignment on the topic and update the PR here by end of next week

@motiz88
Copy link
Collaborator

motiz88 commented Feb 24, 2023

I've spent some time reading this RFC and talking to @cortinico, and I've identified my main issues with it. I'll start with the more focused points, with broader concerns towards the end.

Autocomplete / IDE support

If we add a field to package.json as opposed to a separate file, what are our options for providing a schema that developers can plug into their IDE to get autocompletion etc? Do IDEs like VS Code offer ways to compose extended schemas onto the base package.json schema? (Any DSL we invent should have a good authoring experience as a baseline expectation, so we should design for this even if it's not the top priority.)

Naming

This is a small & bikesheddy point, but I'm gravitating towards reactNativeManifest as the name, inspired by Android manifest files. (And hey, at least I'm not proposing XML. 😅) In my view the term "metadata" can range from human-readable and imprecise to machine-readable and well-defined; the name should convey that this is more the latter, and I feel like "manifest" does it a bit better than "metadata".

Scope creep (current and future)

A "bag of properties" config paradigm can easily end up confusing people by conflating properties that apply at different times, to different scopes, and have different levels of strictness / user-friendliness. In short, we need to ensure this mechanism stays cohesive and easy to learn. We should apply a lot of scrutiny to potential use cases here and lay down stronger guidelines on what's a good fit for reactNativeMetadata vs some other mechanism.

The current proposal introduces reactNativeMetadata with the following description:

This new section will allow developer to express certain characteristics of their code in a more formalized manner, that can easily used by tooling to act towards the code accordingly.

I think that's too open-ended. I'd propose wording to the effect of:

reactNativeMetadata is an optional object in package.json describing specific aspects of the package that are unique to integrating with React Native, which cannot be inferred from standard fields in package.json. It's mainly intended to enable package discovery, autolinking and build-time validation in React Native projects. The metadata is intended to be useful both for published packages (e.g. on npm) and private/workspace packages. The scope of the metadata extends only to files included in the package, excluding any files under ./node_modules.

Concretely:

  • metroExportsMode does not fit here: it's not specific to integrating with React Native (as Metro can also be used outside of React Native). I propose that if we want to create this flag in the future, we leave it outside of reactNativeMetadata
  • requirements seems redundant given that peerDependencies exists (discussion). Even if there are theoretical benefits, they will be hard to teach and maintain. I propose that we remove this and separately explore using peerDependencies (in combination with peerDependenciesMeta?) to solve the same problem.
  • I would suggest that we specify here that the semantics/shape of reactNativeMetadata can only change further via a follow-up RFC (and tool-specific extensions are strictly disallowed).

What is the big picture design?

tl;dr for the purposes of this RFC: we should not block on solving this much larger challenge, BUT we should ruthlessly limit the scope to what's immediately useful so that we can focus future efforts on a more comprehensive solution.

To put it bluntly, it feels like we're dancing around the lack of an end-to-end, user-extensible, designed, build system for native code in React Native projects. We do have a patchwork of systems in place, that many people are using successfully, but the ad hoc nature of these solutions leaves us with a poor caching/incremental builds story, scattered documentation, duplication of tools/effort, wildly differing levels of polish, and - yes - an unclear source of truth for build configuration in a project.

AFAICT, reactNativeMetadata aims to partially address the "source of truth" problem, which is great - but I worry that we may be adopting constraints that wouldn't really work for a more fundamental solution. For example, the use of JSON to express what is essentially build logic (which even includes conditionals, i.e. platforms) seems short-sighted from this perspective. The more complexity we add down the line, the more we will start straining against the lack of Turing-complete facilities and other features of mature build systems. (Or worse: we will start adding them in... badly. See Greenspun's tenth rule.)

Concretely, we should put resources towards designing the overall build system for React Native, and think of our current efforts as the first few steps towards that broader vision.

That cohesive system may or may not be based on Buck; it's much too early to take a firm stance either way. (And frankly this idea might not even materialise soon - for now I'm mainly hoping to start a discussion around it.) But the concept of specifying a single cross-platform graph of build actions, encompassing all the configuration, codegen, compilation, linking, and bundling in a React Native project, is something that Buck makes relatively natural - so I think it's one of the options we should evaluate more closely.

To be clear, there are tough design problems here, e.g. how the unified build system should interoperate with Xcode / Android Studio / and their underlying build systems & package managers - and I'm not claiming to have complete answers at this stage. But there are encouraging precedents at Meta for tools that do glue Buck with Xcode and Android Studio while providing a good and consistent developer experience.

@cortinico cortinico requested review from tido64, NickGerleman and motiz88 and removed request for NickGerleman and tido64 March 31, 2023 16:10
@cortinico
Copy link
Member

Hey all,
I know this RFC was silent for a bit. We reworked it quite substantially to reduce the scope a bit and make it more focused on supporting capabilities. I would be supporting @kelset in driving this forward and I'd like to get another round of review

@brentvatne
Copy link
Contributor

  • requirements seems redundant given that peerDependencies exists (discussion). Even if there are theoretical benefits, they will be hard to teach and maintain. I propose that we remove this and separately explore using peerDependencies (in combination with peerDependenciesMeta?) to solve the same problem.

I want to push back against this point - the peerDependencies field brings a lot of baggage with it, and in the React Native ecosystem it often leads to a bad user experience in the form of unactionable/unhelpful error messages and obscure bugs related to package hoisting or duplicate versions of packages installed, where only a single version actually ever makes sense in the project (eg: for native modules, where only one version can be linked at any given time).

That said, I do appreciate your point about tightening this specification up and I think we could simplify the requirements field by allowing only specific keys instead of arbitrary package names. I think the most important ones to support are react-native and expo. This would allow us to recommend that library authors express peer dependencies on react-native and expo using the * matcher, which indicates they are required but otherwise mostly skips the default package manager peer dependency validation (this validation would break with alpha/beta/rc releases) and delegates those validations to tools that we build instead. We can use this information for a few purposes:

  • Show version compatibility on https://reactnative.directory
  • Validate project dependencies against that data
  • Upgrade package versions automatically when upgrading React Native versions based on that data
  • Installing correct versions of packages for the given React Native / Expo versions

As of today, there is no simple programmatic way to know if a NPM package is a React Native library. Good indicators could be:
* Presence of a `react-native:*` dependency in the `peerDependencies` section of the `package.json` ([example](https://github.com/RonRadtke/react-native-blob-util/blob/80628d418a5c81439dc2c1a1f05fae6406f0ba7f/package.json#L46C10-L49))
* Presence of React Native specific files such as `react-native.config.js` files
Those indicators are all non-exhaustive.

Choose a reason for hiding this comment

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

How do other libraries determine this? I guess it's not really a problem because we need to be able to catalogue libraries specifically for React Native?

Copy link
Member

Choose a reason for hiding this comment

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

How do other libraries determine this?

What do you mean with "other libraries". Like other ecosystems?

Choose a reason for hiding this comment

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

Oops yes other ecosystems. I'm guessing they have similar directories. Are they all maintaining it manually?


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.

Choose a reason for hiding this comment

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

Is this only for apps? I'm guessing it doesn't make sense for a library to specify flags? But I guess there could be a library that enforces a certain feature flag?

Copy link
Member

Choose a reason for hiding this comment

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

It's for both apps and libraries. There could be a scenarios where a capability needs all the dependencies to expose such capability (for example the newArch one).

Choose a reason for hiding this comment

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

Would this proposal enforce this? Like if a library specifies a certain feature flag, and an app uses that library, how do we communicate this?

Copy link
Member

Choose a reason for hiding this comment

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

Would this proposal enforce this? Like if a library specifies a certain feature flag, and an app uses that library, how do we communicate this?

That's up to each feature to define the semantic of such flags. That's also why we need a dedicated RFC for each feature. There might be features that have a hard requirement on dependencies (like the New Architecture Renderer) and other features that have fallbacks

@cortinico cortinico changed the title RFC: introduce reactNativeMetadata to package.json RFC: introduce reactNativeManifest to package.json Apr 6, 2023
@cortinico
Copy link
Member

I want to push back against this point - the peerDependencies field brings a lot of baggage with it, and in the React Native ecosystem it often leads to a bad user experience in the form of unactionable/unhelpful error messages and obscure bugs related to package hoisting or duplicate versions of packages installed, where only a single version actually ever makes sense in the project (eg: for native modules, where only one version can be linked at any given time).

Thanks for flagging this @brentvatne. I think we'll probably have a separate RFC to decide how to proceed with either peerDependencies, requirements or any other mechanism to provide better support for version alignment.

I do personally like your direction of allowing only react-native and expo as it will make it more predictable and easy to validate for users.

@migueldaipre
Copy link

The newArch flag sounds strange to me. For me users new to the React Native ecosystem shouldn't know it as "newArch" or "oldArch".

@cortinico
Copy link
Member

The newArch flag sounds strange to me. For me users new to the React Native ecosystem shouldn't know it as "newArch" or "oldArch".

For context, that's how it's called today:
https://reactnative.dev/docs/new-architecture-app-intro#android---prerequisites
Renaming features is outside the scope of this RFC

Copy link

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

I left some comments. Most of them are fixes for typos, but I left also a couple of questions.

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

Therefore we believe `align-deps` can benefit from this information and be extended to offer New Architecture support information.

### React Native CLI support

Choose a reason for hiding this comment

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

would this be a part of React Native core or a feature we want to defer to frameworks?

Copy link
Member

Choose a reason for hiding this comment

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

This is framework/tooling responsibility. Is added here to provide context on what we believe could be valuable use cases of those APIs

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

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

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

I left some comments. Most of them are fixes for typos, but I left also a couple of questions.

@cipolleschi
Copy link

Hey folks, I created a(nother) PR with some small updates on this.
Let's move the conversation there!

@cipolleschi cipolleschi closed this Oct 6, 2023
@kelset kelset deleted the kelset/rfc-reactNativeMeta branch October 10, 2023 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Proposal This label identifies a proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet