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

Move metro-config package into monorepo build, enable TS generation #41836

Conversation

byCedric
Copy link
Contributor

@byCedric byCedric commented Dec 7, 2023

Summary:

This adds @react-native/metro-config to the monorepo build tool and emits the missing typescript declarations.

Right now, we do have typescript declarations on metro-config, but not @react-native/metro-config. Which makes everything a bit harder extend from "the default React Native metro config" in Expo.

Note, I also added the same exports block from @react-native/dev-middleware for conformity.

One open question here is, why aren't we exporting all helper functions from metro-config? To me, its a bit weird that we need both metro-config and @react-native/metro-config as loadConfig isn't exported.

Changelog:

[INTERNAL] [FIXED] - Emit typescript declaration files for @react-native/metro-config

Test Plan:

Run the build tool, and check if the typescript declarations are emitted for @react-native/metro-config.

@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: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Dec 7, 2023
@byCedric byCedric force-pushed the @bycedric/metro-config/add-type-definitions branch 2 times, most recently from 7033c9f to 1907ea8 Compare December 7, 2023 14:34
Copy link
Member

@huntie huntie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I reckoned Expo was providing its own default config package — but perfectly happy to add TypeScript ergonomics :).

I'm just going to make sure this runs in RNTester — believe we may need to add the wrapping .js/.flow.js pattern to have run-from-source behaviour in this repo / internally at Meta.

@byCedric
Copy link
Contributor Author

byCedric commented Dec 7, 2023

Great! I reckoned Expo was providing its own default config package — but perfectly happy to add TypeScript ergonomics :).

I'm just going to make sure this runs in RNTester — believe we may need to add the wrapping .js/.flow.js pattern to have run-from-source behaviour in this repo / internally at Meta.

We are! But, there are fixes inside this React Native config that aren't backported inside the metro-config package. Things such as these require.resolve blocks.

We keep this "semi-fork" of the default config in @expo/metro-config, but it seems that metro-config should be properly fixed if we want to do that instead. Or am I missing something? 😄

@byCedric
Copy link
Contributor Author

byCedric commented Dec 7, 2023

Also, I think the getDefaultConfig.getDefaultValues should be properly patched as well. Right now, it just overwrites the getDefaultConfig method, without adding getDefaultValues.

@analysis-bot
Copy link

analysis-bot commented Dec 7, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 16,513,663 -17
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 19,884,244 -3
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: a8ca9b0
Branch: main

@byCedric byCedric force-pushed the @bycedric/metro-config/add-type-definitions branch from 1907ea8 to f6c8fd4 Compare December 7, 2023 15:09
@huntie huntie changed the title fix(metro-config): emit typescript declarations for @react-native/metro-config Move metro-config package into monorepo build, enable TS generation Dec 7, 2023
Copy link
Member

@huntie huntie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@facebook-github-bot
Copy link
Contributor

@huntie has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@huntie
Copy link
Member

huntie commented Dec 19, 2023

One open question here is, why aren't we exporting all helper functions from metro-config? To me, its a bit weird that we need both metro-config and @react-native/metro-config as loadConfig isn't exported.

Ah, just spotted this question. The reasoning here is that only mergeConfig is needed inside a consuming metro.config.js file, since it returns an object/function that will be loaded by Metro.

loadConfig is side-effectful and part of Metro's internals (it's ultimately used to read metro.config.js in a project) — it's beyond the scope of the above.

@@ -4,13 +4,14 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow
* @noformat
* @flow strict-local
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah my bad, let's loosen this to @flow so that CI passes.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Feb 12, 2024
@facebook-github-bot
Copy link
Contributor

@huntie merged this pull request in b41a33e.

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. Merged This PR has been merged. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants