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

Incomplete check for @react-native/metro-config #1987

Closed
tido64 opened this issue Jun 26, 2023 · 7 comments
Closed

Incomplete check for @react-native/metro-config #1987

tido64 opened this issue Jun 26, 2023 · 7 comments

Comments

@tido64
Copy link
Contributor

tido64 commented Jun 26, 2023

Environment

n/a

Description

The check for whether @react-native/metro-config is incomplete because metro.config.js is not necessarily self-contained:

if (
!/['"']@react-native\/metro-config['"']/.test(
fs.readFileSync(projectConfig.filepath, 'utf8'),
)
) {

Metro config can be split in several files (or modules) for reusability. The check fails for these scenarios. Example: microsoft/rnx-kit#2481

Reproducible Demo

  1. Instantiate a react-native project with 0.72
  2. Refactor metro.config.js by moving the @react-native/metro-config import to a separate file
  3. Bundle
@tido64
Copy link
Contributor Author

tido64 commented Jun 26, 2023

Suggestion: Can we add a field in @react-native/metro-config that only gets set if it's used instead?

@kelset
Copy link
Member

kelset commented Jun 26, 2023

cc @huntie

@huntie
Copy link
Collaborator

huntie commented Jun 26, 2023

@tido64 Yeah I've considered this. It would require adding an untyped metadata key, e.g. '__metaBaseConfig': '@react-native/metro-config' — which Metro's mergeConfig would not yet handle. Stopped short of this because we strongly recommend a single-file Metro config — but if you have clear/consistent use cases we can consider building this in.

There's also the workaround of a "suppression" comment within (e.g. at the top of) the file:

// Extends '@react-native/metro-config'

Related:

@tido64
Copy link
Contributor Author

tido64 commented Jun 26, 2023

@tido64 Yeah I've considered this. It would require adding an untyped metadata key, e.g. '__metaBaseConfig': '@react-native/metro-config' — which Metro's mergeConfig would not yet handle. Stopped short of this because we strongly recommend a single-file Metro config — but if you have clear/consistent use cases we can consider building this in.

I don't think that will scale as you have more teams working with React Native. At Microsoft, all teams use a shared Metro config unless they have a very good reason not to. We're not going back to having individual teams configuring Metro for themselves.

There's also the workaround of a "suppression" comment within (e.g. at the top of) the file:

// Extends '@react-native/metro-config'

That's currently what we're telling people to do: microsoft/rnx-kit#2481

@huntie
Copy link
Collaborator

huntie commented Jun 29, 2023

@tido64 Planned fix incoming.

huntie added a commit to huntie/react-native that referenced this issue Jun 29, 2023
Summary:
Towards react-native-community/cli#1987. Will be paired with a CLI PR targeting React Native 0.72.1.

Changelog: None

Differential Revision: D47125080

fbshipit-source-id: 17dec929313605fe7a91b1b5c6923ee04bdcf8b9
huntie added a commit to huntie/react-native that referenced this issue Jun 29, 2023
…38126)

Summary:
Pull Request resolved: facebook#38126

Towards react-native-community/cli#1987. Will be paired with a CLI PR targeting React Native 0.72.1.

Changelog: None

Reviewed By: motiz88

Differential Revision: D47125080

fbshipit-source-id: 9c5eb9ed791ace7a1314ebc26e4dcf494cf190e1
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jun 29, 2023
Summary:
Pull Request resolved: #38126

Towards react-native-community/cli#1987. Will be paired with a CLI PR targeting React Native 0.72.1.

Changelog: None

Reviewed By: motiz88

Differential Revision: D47125080

fbshipit-source-id: b3b9d93ba747240f5168021ccb793ffe5d34251d
@huntie
Copy link
Collaborator

huntie commented Jul 3, 2023

Change has landed and should be shipped in React Native 0.72.2.

@huntie huntie closed this as completed Jul 3, 2023
@tido64
Copy link
Contributor Author

tido64 commented Jul 3, 2023

Change has landed and should be shipped in React Native 0.72.2.

Thank you!

kelset pushed a commit to facebook/react-native that referenced this issue Jul 10, 2023
Summary:
Pull Request resolved: #38126

Towards react-native-community/cli#1987. Will be paired with a CLI PR targeting React Native 0.72.1.

Changelog: None

Reviewed By: motiz88

Differential Revision: D47125080

fbshipit-source-id: b3b9d93ba747240f5168021ccb793ffe5d34251d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants