-
Notifications
You must be signed in to change notification settings - Fork 906
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
fix: Fix merge of CLI Metro config overrides, add soft fallback to RN defaults #1896
fix: Fix merge of CLI Metro config overrides, add soft fallback to RN defaults #1896
Conversation
e9267dc
to
3555672
Compare
🚫 Planning changes |
3555672
to
1a4575f
Compare
1a4575f
to
788e421
Compare
✅ Ready for review |
'From React Native 0.72, your metro.config.js file should extend' + | ||
"'@react-native/metro-config'. Please see the React Native 0.72 " + | ||
'changelog, or copy the template at:\n' + | ||
'https://github.com/facebook/react-native/blob/main/packages/react-native/template/metro.config.js', |
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 adding a reference to copy the config
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.
ty, ideal reference is once we have a URL for the changelog/upgrade helper :)
Summary: Changelog: [Internal] Remove internal merge of `getDefaultConfig` (Metro base defaults) from `react-native/metro-config`. This is unnecessary given the config loading setup of RN CLI and Expo CLI, which use (or replicate) Metro's [`loadConfig`](https://github.com/facebook/metro/blob/1e47cb5b3cc289530fb18e402891f9d2816611dd/packages/metro-config/src/loadConfig.js#L182-L190) function — which will itself apply defaults appropriately. This relates to a previously-breaking behaviour documented in the test plan of react-native-community/cli#1896 (independently fixed and no longer load-bearing) (**read: no need to cherry pick this change**). https://pxl.cl/2B8NS While this has no effect under the fixed RN CLI setup, this is a worthwhile simplification to this package that better-aligns with current Metro tooling expectations. ## Notes - `getDefaultConfig` no longer returns `ConfigT` (full config), and instead returns `MetroConfig` (partial config). This is non-breaking with the expected API of a given `metro.config.js` file. Differential Revision: D44630645 fbshipit-source-id: c8e36f1e2faa8f76ec2986586657027923b0dbbb
Summary: Pull Request resolved: facebook#36777 Changelog: [Internal] Remove internal merge of `getDefaultConfig` (Metro base defaults) from `react-native/metro-config`. This is unnecessary given the config loading setup of RN CLI and Expo CLI, which use (or replicate) Metro's [`loadConfig`](https://github.com/facebook/metro/blob/1e47cb5b3cc289530fb18e402891f9d2816611dd/packages/metro-config/src/loadConfig.js#L182-L190) function — which will itself apply defaults appropriately. This relates to a previously-breaking behaviour documented in the test plan of react-native-community/cli#1896 (independently fixed and no longer load-bearing) (**read: no need to cherry pick this change**). https://pxl.cl/2B8NS While this has no effect under the fixed RN CLI setup, this is a worthwhile simplification to this package that better-aligns with current Metro tooling expectations. ## Notes - `getDefaultConfig` no longer returns `ConfigT` (full config), and instead returns `MetroConfig` (partial config). This is non-breaking with the expected API of a given `metro.config.js` file. Differential Revision: D44630645 fbshipit-source-id: e85247b145e87d7bac9eca0bcb90e85e6fec98ae
Summary: Pull Request resolved: facebook#36777 Changelog: [Internal] Remove internal merge of `getDefaultConfig` (Metro base defaults) from `react-native/metro-config`. This is unnecessary given the config loading setup of RN CLI and Expo CLI, which use (or replicate) Metro's [`loadConfig`](https://github.com/facebook/metro/blob/1e47cb5b3cc289530fb18e402891f9d2816611dd/packages/metro-config/src/loadConfig.js#L182-L190) function — which will itself apply defaults appropriately. This relates to a previously-breaking behaviour documented in the test plan of react-native-community/cli#1896 (independently fixed and no longer load-bearing) (**read: no need to cherry pick this change**). https://pxl.cl/2B8NS While this has no effect under the fixed RN CLI setup, this is a worthwhile simplification to this package that better-aligns with current Metro tooling expectations. ## Notes - `getDefaultConfig` no longer returns `ConfigT` (full config), and instead returns `MetroConfig` (partial config). This is non-breaking with the expected API of a given `metro.config.js` file. Differential Revision: D44630645 fbshipit-source-id: fa2c9f81767659f1c4fa5a8abf267173b182b3d9
Summary: Pull Request resolved: #36777 Changelog: [Internal] Remove internal merge of `getDefaultConfig` (Metro base defaults) from `react-native/metro-config`. This is unnecessary given the config loading setup of RN CLI and Expo CLI, which use (or replicate) Metro's [`loadConfig`](https://github.com/facebook/metro/blob/1e47cb5b3cc289530fb18e402891f9d2816611dd/packages/metro-config/src/loadConfig.js#L182-L190) function — which will itself apply defaults appropriately. This relates to a previously-breaking behaviour documented in the test plan of react-native-community/cli#1896 (independently fixed and no longer load-bearing) (**read: no need to cherry pick this change**). https://pxl.cl/2B8NS While this has no effect under the fixed RN CLI setup, this is a worthwhile simplification to this package that better-aligns with current Metro tooling expectations. ## Notes - `getDefaultConfig` no longer returns `ConfigT` (full config), and instead returns `MetroConfig` (partial config). This is non-breaking with the expected API of a given `metro.config.js` file. Reviewed By: cipolleschi Differential Revision: D44630645 fbshipit-source-id: 472c3967449dfb99f845a82d9e9c49efc343021c
Summary: Pull Request resolved: facebook#36777 Changelog: [Internal] Remove internal merge of `getDefaultConfig` (Metro base defaults) from `react-native/metro-config`. This is unnecessary given the config loading setup of RN CLI and Expo CLI, which use (or replicate) Metro's [`loadConfig`](https://github.com/facebook/metro/blob/1e47cb5b3cc289530fb18e402891f9d2816611dd/packages/metro-config/src/loadConfig.js#L182-L190) function — which will itself apply defaults appropriately. This relates to a previously-breaking behaviour documented in the test plan of react-native-community/cli#1896 (independently fixed and no longer load-bearing) (**read: no need to cherry pick this change**). https://pxl.cl/2B8NS While this has no effect under the fixed RN CLI setup, this is a worthwhile simplification to this package that better-aligns with current Metro tooling expectations. ## Notes - `getDefaultConfig` no longer returns `ConfigT` (full config), and instead returns `MetroConfig` (partial config). This is non-breaking with the expected API of a given `metro.config.js` file. Reviewed By: cipolleschi Differential Revision: D44630645 fbshipit-source-id: 472c3967449dfb99f845a82d9e9c49efc343021c
Summary: Pull Request resolved: facebook#36777 Changelog: [Internal] Remove internal merge of `getDefaultConfig` (Metro base defaults) from `react-native/metro-config`. This is unnecessary given the config loading setup of RN CLI and Expo CLI, which use (or replicate) Metro's [`loadConfig`](https://github.com/facebook/metro/blob/1e47cb5b3cc289530fb18e402891f9d2816611dd/packages/metro-config/src/loadConfig.js#L182-L190) function — which will itself apply defaults appropriately. This relates to a previously-breaking behaviour documented in the test plan of react-native-community/cli#1896 (independently fixed and no longer load-bearing) (**read: no need to cherry pick this change**). https://pxl.cl/2B8NS While this has no effect under the fixed RN CLI setup, this is a worthwhile simplification to this package that better-aligns with current Metro tooling expectations. ## Notes - `getDefaultConfig` no longer returns `ConfigT` (full config), and instead returns `MetroConfig` (partial config). This is non-breaking with the expected API of a given `metro.config.js` file. Reviewed By: cipolleschi Differential Revision: D44630645 fbshipit-source-id: 472c3967449dfb99f845a82d9e9c49efc343021c
Context
React Native Metro config → React Native repo (facebook/react-native#36502)
Follow ups to #1875.
Changes
Fixes
Critical fix to the order in which Metro config sets are merged.
loadConfig({cwd: ctx.root, ...options}, overrideConfig);
(see original file diff) did not correctly mergeoverrideConfig
(the extra config applied by RN CLI) over the newly migrated defaults (which were promoted into the project-level config file).overrideConfig
will now be merged after all config, including any user-set config. This makes sense (as before), given the CLI applies very sparse override options based on required CLI inputs.Upgrade improvements
Provide a soft upgrade path for users on React Native 0.72 to migrate to the new
metro.config.js
setup in 0.73.loadMetroConfig
implementation to conditionally fall back to internal defaults. The intent is to remove this behaviour (and dependency in CLI) for React Native 0.73.@react-native/metro-config
source.metro.config.js
source.Changelog: Fix bug where CLI-overridden Metro config might not have been applied
📦 Please publish!: We'd like to integrate this change into the 0.72 RC1 early next week. Apologies for the churn 🙏🏻.
Test Plan
Using
rn-tester
configured with a locallyyarn link
ed copy of RN CLI.✅ In all test cases, I added temporary debug logs for the entire final config object, which were compared and produce no diff between either config setup.
New 0.72 config setup (project extends
@react-native/metro-config
)✅ App (rn-tester) runs with no CLI warnings
✅ Correctly merged
overrideConfig
In the new config setup, the CLI
overrideConfig
was previously clobbered by the project-levelmetro.config.js
which broke new template apps. This is now fixed.FIXED:
serializer.getModulesRunBeforeMainModule
now overrides the default value inmetro-config
(metro), cc @cipolleschiFIXED:
resolver.platforms
now overrides the default value in@react-native/metro-config
.Final config in RN CLI compared to before this PR:
(
@react-native/metro-config
uses an intentionally simpler default for this option. The former value will be read by standalone Metro CLI, and the latter is overridden by RN CLI based onctx.platforms
).Fallback 0.71 config setup (project does not extend
@react-native/metro-config
)✅ Warning and fallback config behaviour are present
✅ Correctly merged
overrideConfig
(As in previous test case. Entire object was also checked.)