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

Restore base config merge in metro-config #38092

Closed
wants to merge 1 commit into from

Conversation

huntie
Copy link
Member

@huntie huntie commented Jun 27, 2023

Summary

Reverts #36777.

This is motivated by reducing user friction when the widespread assumption is for @react-native/metro-config to export a complete Metro config, as with Expo/rnx-kit, and like previously understood uses of metro-config. See facebook/metro#1010 (comment) for further notes.

Fixes:

Note that we do not intend for @react-native/metro-config to directly export assetExts etc — these can be accessed on the resolver property from the full config object.

Changelog: [General][Changed] @react-native/metro-config now includes all base config values from metro-config

Test Plan

Steps

  • Using a new React Native 0.72 project.
    • Use yarn link to substitute @react-native/metro-config with local version.
    • Use yarn link to substitute @react-native-community/cli-plugin-metro with local fork.
      • This is modified to log the final config to the terminal.
    • Expand watchFolders in Metro config.
    • yarn start

Produces functionally identical final config

(Diff before and after changes)

App builds

image

Differential Revision: D47055973

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner fb-exported labels Jun 27, 2023
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D47055973

@huntie
Copy link
Member Author

huntie commented Jun 27, 2023

@kelset @cipolleschi @hoxyq Do you know if package version bumps are performed as part of the release process? Or should I increment the version here (to be subsequently bumped in the template)?

@analysis-bot
Copy link

analysis-bot commented Jun 27, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,045,226 -1
android hermes armeabi-v7a 8,294,498 -1
android hermes x86 9,561,418 -2
android hermes x86_64 9,403,753 +0
android jsc arm64-v8a 9,604,418 -1
android jsc armeabi-v7a 8,731,133 -1
android jsc x86 9,691,465 +0
android jsc x86_64 9,937,808 +0

Base commit: c803a5b
Branch: main

@kelset
Copy link
Contributor

kelset commented Jun 27, 2023

@kelset @cipolleschi @hoxyq Do you know if package version bumps are performed as part of the release process? Or should I increment the version here (to be subsequently bumped in the template)?

yes they are done as part of the release process, please don't bump here.

@kelset
Copy link
Contributor

kelset commented Jun 27, 2023

separate question: how does this change affect existing users on 0.72? will everything keep working as expected, or would they need to change anything?

'cause that will affect if we can cherry-pick in 0.72.

@huntie
Copy link
Member Author

huntie commented Jun 27, 2023

@kelset Ah yeah of course, ty 👍🏻

@robhogan
Copy link
Contributor

separate question: how does this change affect existing users on 0.72? will everything keep working as expected, or would they need to change anything?
'cause that will affect if we can cherry-pick in 0.72.

This is non-breaking as I see it, so should be fine to pick - we'll just be exporting a complete config object rather than a loose, partial one.

Summary:
Pull Request resolved: facebook#38092

Reverts facebook#36777.

This is motivated by reducing user friction when the widespread assumption is for `react-native/metro-config` to export a complete Metro config, as with Expo/rnx-kit, and like previously understood uses of `metro-config`. See facebook/metro#1010 (comment) for further notes.

Fixes:
- facebook/metro#1010
- facebook#38069
- kristerkari/react-native-svg-transformer#276

Note that we do not intend for `react-native/metro-config` to directly export `assetExts` etc — these can be accessed on the `resolver` property from the full config object.

Changelog: [General][Changed] `react-native/metro-config` now includes all base config values from `metro-config`

Reviewed By: robhogan

Differential Revision: D47055973

fbshipit-source-id: eedc4698e651645ada46a013d3945a16965bff22
@huntie huntie force-pushed the export-D47055973 branch from ac5d178 to 7e5af62 Compare June 28, 2023 10:51
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D47055973

@github-actions
Copy link

This pull request was successfully merged by @huntie in bbcedd3.

When will my fix make it into a release? | Upcoming Releases

@github-actions github-actions bot added the Merged This PR has been merged. label Jun 28, 2023
kelset pushed a commit that referenced this pull request Jun 28, 2023
Summary:
Pull Request resolved: #38092

Reverts #36777.

This is motivated by reducing user friction when the widespread assumption is for `react-native/metro-config` to export a complete Metro config, as with Expo/rnx-kit, and like previously understood uses of `metro-config`. See facebook/metro#1010 (comment) for further notes.

Fixes:
- facebook/metro#1010
- #38069
- kristerkari/react-native-svg-transformer#276

Note that we do not intend for `react-native/metro-config` to directly export `assetExts` etc — these can be accessed on the `resolver` property from the full config object.

Changelog: [General][Changed] `react-native/metro-config` now includes all base config values from `metro-config`

Reviewed By: robhogan

Differential Revision: D47055973

fbshipit-source-id: 5ad23cc9700397110de5c0e81c7d76299761ef0a
@huntie huntie deleted the export-D47055973 branch August 29, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants