feat: add `globalPrefix` to remaining require polyfill methods #1512

jbroma · 2025-06-04T17:15:40Z

Summary

Part of #1480

This PR adds __METRO_GLOBAL_PREFIX__ to other require polyfill methods. With Module Federation we use multiple module system in runtime which creates a need for separating module systems from each other with globalPrefix.

Changelog: [Feature] Add global prefix to other module functions in require polyfill

Test plan

TBD

jbroma · 2025-06-05T11:18:45Z

Fix test to use __customPrefix__r

on the other hand, we discussed that it might not be needed to add the prefix to __r but after closer inspection it turns out it's necessary - I'm wondering whether we should introduce a separate prefix for __r or keep it as it is in this PR (prefixed with __METRO_GLOBAL_PREFIX__)?

jbroma · 2025-06-27T17:59:53Z

Fix test to use __customPrefix__r

on the other hand, we discussed that it might not be needed to add the prefix to __r but after closer inspection it turns out it's necessary - I'm wondering whether we should introduce a separate prefix for __r or keep it as it is in this PR (prefixed with __METRO_GLOBAL_PREFIX__)?

cc @robhogan do you think we should create a separate prefix for __r?

We've added tests to the PR and marked it as ready to review, let me know if there is anything left to be done here to proceed with this PR 🙏

packages/metro-runtime/src/polyfills/__tests__/require-test.js

robhogan · 2025-07-02T12:37:17Z

We'll need to update this as well so users with a custom global prefix but default getRunModuleStatement don't get broken:

metro/packages/metro-config/src/defaults/index.js

Lines 63 to 64 in 35f2e48

    
           getRunModuleStatement: (moduleId: number | string) => 
        
             `__r(${JSON.stringify(moduleId)});`,

robhogan · 2025-07-02T12:37:50Z

LGTM in principle, importing to run internal tests

facebook-github-bot · 2025-07-02T12:38:17Z

@robhogan has imported this pull request. If you are a Meta employee, you can view this in D77659025.

jbroma · 2025-07-02T13:19:56Z

We'll need to update this as well so users with a custom global prefix but default getRunModuleStatement don't get broken:

metro/packages/metro-config/src/defaults/index.js

Lines 63 to 64 in 35f2e48

getRunModuleStatement: (moduleId: number | string) =>

`__r(${JSON.stringify(moduleId)});`,

there is 2 ways to achieve this:

we can modify the returned statement to do some runtime interpolation and include the __METRO_GLOBAL_PREFIX__ from current scope
we can augument the getRunModuleStatement with another param that will insert the prefix at build time

what would you suggest here?

jbroma · 2025-07-29T09:34:49Z

@robhogan any suggestions on how to move forward with this PR?

…Statement` Summary: To allow prefixing the global function `__r` (e.g., #1512), expose the prefix to the configured [`getRunModuleStatement`](https://metrobundler.dev/docs/configuration/#getrunmodulestatement), which returns the `__r()` call as a string. ``` - **[Feature]** Expose `globalPrefix` to `getRunModuleStatement` ``` ## Alternative? Runtime prefixing We could alternatively add prefix at runtime by emitting: ``` 'globalThis[__METRO_GLOBAL_PREFIX__ + '__r'](/*...*/)' ``` There are a couple of downsides - - These runModule statements are currently outside IIFEs that inject `global` based on (something like) `global = globalThis ?? global ?? window` - we don't rely on the existence of `globalThis` elsewhere (though it's probably safe to now - that's a separate breaking change). - Concatenation/interpolation + object access is more verbose and slightly slower, and there's just no need for it in code emitted by Metro itself (as opposed to framework/userland code, which must use it). Differential Revision: D81476621

robhogan · 2025-09-02T10:39:09Z

Ideally I think we add the prefix at build time - should be possible to add on top of #1566

…Statement` (#1566) Summary: Pull Request resolved: #1566 To allow prefixing the global function `__r` (e.g., #1512), expose the prefix to the configured [`getRunModuleStatement`](https://metrobundler.dev/docs/configuration/#getrunmodulestatement), which returns the `__r()` call as a string. ``` - **[Feature]** Expose `globalPrefix` to `getRunModuleStatement` ``` ## Alternative? Runtime prefixing We could alternatively add prefix at runtime by emitting: ``` "globalThis[__METRO_GLOBAL_PREFIX__ + '__r'](/*...*/)" ``` There are a couple of downsides - - These runModule statements are currently outside IIFEs that inject `global` based on (something like) `global = globalThis ?? global ?? window` - we don't rely on the existence of `globalThis` elsewhere (though it's probably safe to now - that's a separate breaking change). - Concatenation/interpolation + object access is more verbose and slightly slower, and there's just no need for it in code emitted by Metro itself (as opposed to framework/userland code, which must use it). Reviewed By: huntie Differential Revision: D81476621 fbshipit-source-id: 90641cf21491711c56606131ec4c5797b7b00020

facebook-github-bot · 2025-09-02T13:55:28Z

@robhogan has imported this pull request. If you are a Meta employee, you can view this in D77659025.

facebook-github-bot · 2025-09-02T13:55:37Z

@robhogan has imported this pull request. If you are a Meta employee, you can view this in D77659025.

jbroma · 2025-09-08T15:01:39Z

I believe that because of changes in #1566 we should also change the default to include the __METRO_GLOBAL_PREFIX__, in a separate PR though

robhogan · 2025-09-09T23:03:06Z

Yes, but tbh I think these need to be the same PR or at least merged simultaneously - users may have already configured globalPrefix, but if so with the default getRunModuleStatement they'd be broken by this PR. The two together is non-breaking for a given config that customises globalPrefix. (Unavoidably breaking for users customising getRunModuleStatement)

(Related, but separately, does anyone need to customise getRunModuleStatement? I wouldn't remove it immediately but I'm inclined to deprecate it)

CC @tjzel - IIRC worklets rely on the details here.

jbroma · 2025-09-10T08:17:37Z

Yes, but tbh I think these need to be the same PR or at least merged simultaneously - users may have already configured globalPrefix, but if so with the default getRunModuleStatement they'd be broken by this PR. The two together is non-breaking for a given config that customises globalPrefix. (Unavoidably breaking for users customising getRunModuleStatement)

yeah, let's aim to merge those 2 one after another 👍

tjzel · 2025-09-10T08:58:37Z

Thanks for the ping.

We don't use getRunModuleStatement in Worklets now. We probably could to make some code less hacky but that would be only a minor improvement.

Just to make sure, after these changes, to obtain the __r function from JSI in C++ we have to make the following amendment?

- auto require = rt.global().getPropertyAsFunction(rt, "__r");
+ auto metroGlobalPrefix = rt.global().getProperty(rt, "__METRO_GLOBAL_PREFIX__");
+ auto requireName = metroGlobalPrefix + "__r";
+ auto require = rt.global().getPropertyAsFunction(rt, requireName);

jbroma · 2025-09-10T16:01:58Z

Just to make sure, after these changes, to obtain the __r function from JSI in C++ we have to make the following amendment?
- auto require = rt.global().getPropertyAsFunction(rt, "__r");
+ auto metroGlobalPrefix = rt.global().getProperty(rt, "__METRO_GLOBAL_PREFIX__");
+ auto requireName = metroGlobalPrefix + "__r";
+ auto require = rt.global().getPropertyAsFunction(rt, requireName);

Correct 👍

I had a quick look but it seems to be one of the only uses of that in OSS 😅
Very nice this was spotted so soon!

feat: add global prefix to other require primitives

2a37345

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 4, 2025

This comment was marked as spam.

Sign in to view

Esemesek and others added 3 commits June 24, 2025 12:31

fix: update and fix unit tests

4cb9122

fix: test typo

0baf33f

Merge branch 'main' into feat/global-prefixes

d983d7a

jbroma marked this pull request as ready for review June 27, 2025 17:58

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jun 27, 2025

jbroma added 2 commits June 29, 2025 14:17

Merge branch 'main' into feat/global-prefixes

ba332cd

Merge branch 'main' into feat/global-prefixes

fd9d6f2

robhogan reviewed Jul 2, 2025

View reviewed changes

packages/metro-runtime/src/polyfills/__tests__/require-test.js Outdated Show resolved Hide resolved

refactor: add __customPrefix to __r

66b9304

robhogan mentioned this pull request Sep 2, 2025

Pass globalPrefix through to serializer and expose to getRunModuleStatement #1566

Closed

robhogan added this to the Next breaking release milestone Sep 4, 2025

Merge remote-tracking branch 'origin/main' into feat/global-prefixes

fa74115

jbroma mentioned this pull request Sep 8, 2025

Include globalPrefix with the default getRunModuleStatement #1571

Open

1 task

feat: add globalPrefix to remaining require polyfill methods #1512

Are you sure you want to change the base?

feat: add globalPrefix to remaining require polyfill methods #1512

Conversation

jbroma commented Jun 4, 2025

Summary

Test plan

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

jbroma commented Jun 5, 2025

Uh oh!

jbroma commented Jun 27, 2025

Uh oh!

Uh oh!

robhogan commented Jul 2, 2025

Uh oh!

robhogan commented Jul 2, 2025

Uh oh!

facebook-github-bot commented Jul 2, 2025

Uh oh!

jbroma commented Jul 2, 2025

Uh oh!

jbroma commented Jul 29, 2025

Uh oh!

robhogan commented Sep 2, 2025

Uh oh!

facebook-github-bot commented Sep 2, 2025

Uh oh!

facebook-github-bot commented Sep 2, 2025

Uh oh!

jbroma commented Sep 8, 2025

Uh oh!

robhogan commented Sep 9, 2025

Uh oh!

jbroma commented Sep 10, 2025

Uh oh!

tjzel commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbroma commented Sep 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

feat: add `globalPrefix` to remaining require polyfill methods #1512

feat: add `globalPrefix` to remaining require polyfill methods #1512

tjzel commented Sep 10, 2025 •

edited

Loading