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

fix: Fix merge of CLI Metro config overrides, add soft fallback to RN defaults #1896

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

huntie
Copy link
Collaborator

@huntie huntie commented Mar 31, 2023

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.

  • Fix a subtly introduced bug where loadConfig({cwd: ctx.root, ...options}, overrideConfig); (see original file diff) did not correctly merge overrideConfig (the extra config applied by RN CLI) over the newly migrated defaults (which were promoted into the project-level config file).
    • After changes, 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.
    • See test plan for E2E bugs fixed.

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.

  • Update the CLI's 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.
    • This conceptually brings the implementation back to the first version of the original PR where we had "getDefaultConfigPre72". Instead, this now depends on the common @react-native/metro-config source.
  • Improve warning guidance to reference template metro.config.js source.
    • Note: This may be later replaced with a React Native changelog deep link, once published.

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 locally yarn linked 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

image

✅ Correctly merged overrideConfig

In the new config setup, the CLI overrideConfig was previously clobbered by the project-level metro.config.js which broke new template apps. This is now fixed.

FIXED: serializer.getModulesRunBeforeMainModule now overrides the default value in metro-config (metro), cc @cipolleschi

image

FIXED: resolver.platforms now overrides the default value in @react-native/metro-config.

Final config in RN CLI compared to before this PR:

- platforms: [ 'android', 'ios' ],
+ platforms: [ 'ios', 'android', 'native' ],

(@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 on ctx.platforms).

Fallback 0.71 config setup (project does not extend @react-native/metro-config)

✅ Warning and fallback config behaviour are present

image

✅ Correctly merged overrideConfig

image

(As in previous test case. Entire object was also checked.)

@huntie huntie changed the title fix: Allow soft fallback to internal RN Metro defaults fix: Fix merge of CLI-applied Metro config, add soft fallback to RN defaults Mar 31, 2023
@huntie huntie force-pushed the metro-mergeconfig-fix branch from e9267dc to 3555672 Compare March 31, 2023 17:28
@huntie huntie marked this pull request as ready for review March 31, 2023 18:08
@huntie huntie requested review from adamTrz and thymikee as code owners March 31, 2023 18:08
@huntie
Copy link
Collaborator Author

huntie commented Mar 31, 2023

🚫 Planning changes

@huntie huntie force-pushed the metro-mergeconfig-fix branch from 3555672 to 1a4575f Compare April 3, 2023 11:00
@huntie huntie changed the title fix: Fix merge of CLI-applied Metro config, add soft fallback to RN defaults fix: Allow soft fallback to internal RN Metro defaults Apr 3, 2023
@huntie huntie force-pushed the metro-mergeconfig-fix branch from 1a4575f to 788e421 Compare April 3, 2023 13:14
@huntie huntie changed the title fix: Allow soft fallback to internal RN Metro defaults fix: Fix merge of CLI Metro config overrides, add soft fallback to RN defaults Apr 3, 2023
@huntie
Copy link
Collaborator Author

huntie commented Apr 3, 2023

✅ Ready for review

Comment on lines +108 to +111
'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',
Copy link
Member

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

Copy link
Collaborator Author

@huntie huntie Apr 3, 2023

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 :)

@thymikee thymikee merged commit 6c1096b into react-native-community:main Apr 3, 2023
huntie added a commit to huntie/react-native that referenced this pull request Apr 3, 2023
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
huntie added a commit to huntie/react-native that referenced this pull request Apr 18, 2023
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
huntie added a commit to huntie/react-native that referenced this pull request Apr 18, 2023
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
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 18, 2023
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
jeongshin pushed a commit to jeongshin/react-native that referenced this pull request May 7, 2023
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
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
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
@huntie huntie deleted the metro-mergeconfig-fix branch May 25, 2023 14:13
huntie added a commit to huntie/react-native-cli that referenced this pull request Jun 1, 2023
huntie added a commit to huntie/react-native-cli that referenced this pull request Jun 15, 2023
huntie added a commit to huntie/react-native-cli that referenced this pull request Jun 15, 2023
huntie added a commit to huntie/react-native-cli that referenced this pull request Jun 16, 2023
huntie added a commit to huntie/react-native-cli that referenced this pull request Jun 16, 2023
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.

2 participants