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

feat(metro-runtime): support using strings as module IDs #1132

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

EvanBacon
Copy link
Contributor

@EvanBacon EvanBacon commented Nov 5, 2023

Summary

Using strings will make it easier to debug larger bundles by allowing users to optionally preserve module names with something like serializer.createModuleIdFactory.

Unclear how to support the nativeRequire code since it's unclear how it works, there's some sort of bit shift being performed on the numeric IDs.

Probably also want to drop verbose module IDs since you can just use the string module IDs.

Test plan

TBD. Unclear if this fully works due to features that don't have comments or any usage in the repo.

I also can't get vs code to respect the flow types used in this file so I build it outside the repo and copy the changes in.

Screenshot 2023-11-05 at 2 44 57 PM

@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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Nov 5, 2023
EvanBacon added a commit to expo/expo that referenced this pull request Jul 9, 2024
# Why

- In React Server Components, we need deterministic module IDs since the
bundles are not created together in development.
- Metro's default require implementation only uses non-deterministic
numeric IDs which is not just broken but it's also really hard to debug
when a module is missing.
- This PR brings over proposed changes from
facebook/metro#1132

<!--
Please describe the motivation for this PR, and link to relevant GitHub
issues, forums posts, or feature requests.
-->

# How

- Fork the metro require implementation and hide it behind a flag
`EXPO_USE_METRO_REQUIRE`.
- Drop support for the require name hints since they assume that the
module ID cannot be a string.
- Create a custom build pipeline in CLI to support the special
requirements of the metro runtime require module (cannot have module
interop).

# Test Plan

- Forked the tests too so we can ensure parity with the standard system.
- Added a new E2E web test to ensure the new bundles are runnable.

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
alanjhughes pushed a commit to expo/expo that referenced this pull request Jul 17, 2024
# Why

- In React Server Components, we need deterministic module IDs since the
bundles are not created together in development.
- Metro's default require implementation only uses non-deterministic
numeric IDs which is not just broken but it's also really hard to debug
when a module is missing.
- This PR brings over proposed changes from
facebook/metro#1132

<!--
Please describe the motivation for this PR, and link to relevant GitHub
issues, forums posts, or feature requests.
-->

# How

- Fork the metro require implementation and hide it behind a flag
`EXPO_USE_METRO_REQUIRE`.
- Drop support for the require name hints since they assume that the
module ID cannot be a string.
- Create a custom build pipeline in CLI to support the special
requirements of the metro runtime require module (cannot have module
interop).

# Test Plan

- Forked the tests too so we can ensure parity with the standard system.
- Added a new E2E web test to ensure the new bundles are runnable.

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
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. 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.

None yet

2 participants