-
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
WIP: Metro ESM support proposal #634
Draft
motiz88
wants to merge
3
commits into
react-native-community:main
Choose a base branch
from
motiz88:metro-esm-support
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
--- | ||
title: Improved support for ECMAScript modules | ||
author: | ||
- Moti Zilberman | ||
date: 2023-04-17 | ||
--- | ||
|
||
# RFC0634: Improved support for ECMAScript modules | ||
|
||
## Summary | ||
|
||
Enabling built-in support for ECMAScript modules in Metro for all React Native projects, fixing limitations in the current setup. | ||
|
||
## Motivation | ||
|
||
Today, [`metro-react-native-babel-preset`](https://github.com/facebook/metro/tree/main/packages/metro-react-native-babel-preset) includes the standard [Babel ESM-->CommonJS plugin](https://reactnative.dev/docs/ram-bundles-inline-requires#inline-requires) which compiles `import`, `export` etc into `require()`. | ||
|
||
There are a few problems in this model: | ||
|
||
* **Problem 1:** [Inline requires](https://reactnative.dev/docs/ram-bundles-inline-requires#inline-requires) do not work reliably for `import` declarations ([metro#909](https://github.com/facebook/metro/issues/909)). There is a separate, undocumented preset option, `lazyImportExportTransform`, that controls `import` inlining via the Babel plugin's [`lazy`](https://babeljs.io/docs/babel-plugin-transform-modules-commonjs#lazy) option. | ||
* **Problem 2:** `Platform` inlining doesn't work for the common case of `import {Platform} from 'react-native'` ([metro#800](https://github.com/facebook/metro/pull/800#issuecomment-1092434015)). | ||
* **Problem 3:** [Lazy bundling](https://github.com/react-native-community/discussions-and-proposals/blob/main/proposals/0605-lazy-bundling.md) does not work, since Babel transforms away `import()` calls before Metro can process them. | ||
|
||
These problems lead to larger bundle sizes, worse runtime performance, and slow development speed. | ||
|
||
The unstable `experimentalImportSupport: true` flag in Metro enables built-in ESM support similar to Babel's (notably, with the same `__esModule: true` interop) that solves the above problems. This has been in use at Meta for the last few years. This proposal aims to make this the default behaviour in Metro in a way that is least disruptive. | ||
|
||
Any change to the semantics and ordering of module initialisation is theoretically breaking, so this proposal also includes an opt-out mechanism and warnings when the build is misconfigured. | ||
|
||
## Detailed design | ||
|
||
### Core changes | ||
|
||
The `experimentalImportSupport` flag will be removed and have no effect on Metro's output. | ||
|
||
The `inlineRequires: true` flag will remain in the default React Native [Metro config](https://github.com/facebook/react-native/blob/f5c060618584ae736987b48557d69c501547f9d4/packages/metro-config/index.js#L73). | ||
|
||
`@babel/plugin-transform-modules-commonjs` will be removed from `metro-react-native-babel-preset`, and with it the `lazyImportExportTransform` and `disableImportExportTransform` preset options, which will no longer have an effect on the preset's output. | ||
|
||
> _Alternative_: Rename `disableImportExportTransform` to `enableImportExportTransform` with a default of `false`. | ||
|
||
Metro will always apply its built-in ESM transform after any user-specified Babel plugins. If the user's Babel config includes a different ESM transform, Metro's built-in one will have no effect. | ||
|
||
In the course of implementing this, we will expand the test coverage for Metro's built-in ESM transform to ensure parity and compatibility with Babel's version. | ||
|
||
As a result of these changes, we will no longer need to dynamically set `disableImportExportTransform` from `metro-react-native-babel-transformer` when enabling the built-in ESM transform, which is a brittle mechanism that users can easily override accidentally today. | ||
|
||
### Opt-out and warnings | ||
|
||
In case the new `import` inlining causes issues, users can revert to the old behaviour by manually adding `@babel/plugin-transform-modules-commonjs` to their Babel configs. Note that it may not be the *exact* old behaviour (because Metro currently has [custom defaults](https://github.com/facebook/metro/blob/main/packages/metro-react-native-babel-preset/src/configs/main.js#L66-L68) for the `lazy` Babel option), but it will be *possible* to configure the plugin to closely model the old behaviour as needed. | ||
|
||
For most users, a better option than reverting will be to blocklist specific modules from the inlining behaviour, or better yet, fix the reliance on implicit module side effects. We will provide easy-to-follow guidance and include it in the release announcement. | ||
|
||
If any of the removed config flags (`experimentalImportSupport`, `disableImportExportTransform` and `lazyImportExportTransform`) are detected, Metro will show warnings in the terminal with relevant instructions on how to migrate the Metro config correctly. The flags will not have an effect on Metro's output. | ||
|
||
If Metro detects that a user-specified Babel plugin has removed all `import` and `export` AST nodes from a module, it will show a warning in the terminal recommending that the user remove the plugin and use the built-in support for ESM. | ||
|
||
The warnings described above will only be shown once per instance of Metro, to avoid spamming the terminal. | ||
|
||
|
||
## Drawbacks | ||
|
||
Why should we _not_ do this? Please consider: | ||
|
||
- implementation cost, both in term of code size and complexity | ||
- whether the proposed feature can be implemented in user space | ||
- the impact on teaching people React Native | ||
- integration of this feature with other existing and planned features | ||
- cost of migrating existing React Native applications (is it a breaking change?) | ||
|
||
There are tradeoffs to choosing any path. Attempt to identify them here. | ||
|
||
## Alternatives | ||
|
||
Inlining `require` calls and `import`s is nonstandard behaviour. There's an argument to be made for migrating to a system akin to Webpack's tree shaking implementation, which has fine-grained hints about whether expressions and modules are side-effect free. Such a system would take longer to build and roll out, and might require more migration effort in common cases. | ||
|
||
We could also disable all `require` and `import` inlining by default, but this would likely be a performance regression in common cases. | ||
|
||
## Adoption strategy | ||
|
||
If we implement this proposal, how will existing React Native developers adopt it? Is this a breaking change? Can we write a codemod? Should we coordinate with other projects or libraries? | ||
|
||
TODO: Jest + preset changes | ||
|
||
## How we teach this | ||
|
||
What names and terminology work best for these concepts and why? How is this idea best presented? As a continuation of existing React patterns? | ||
|
||
Would the acceptance of this proposal mean the React Native documentation must be re-organized or altered? Does it change how React Native is taught to new developers at any level? | ||
|
||
How should this feature be taught to existing React Native developers? | ||
|
||
## Unresolved questions | ||
|
||
Optional, but suggested for first drafts. What parts of the design are still TBD? |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 mentioned this in person but just to record it here - for
import()
specifically, could we theoretically preserve those (which are obviously "inline" already) to unblock lazy bundling without changing our approach top levelimport
?