facebook
feat: don't override async bundle paths searchParams in prod
Summary
Part of #1480
When implementing Module Federation for Metro, we're reusing existing lazy bundling for production bundles, and we wanted to clean up the bundlePaths for the production bundles by not adding any query params to it. Since the searchParams are entirely based on sourceUrl, we can avoid adding searchParams by having a clean sourceUrl.
This PR does two things:
- [x] - it makes the override of
runModule&modulesOnlyparams happen only in dev - [x] - prevents adding
?to the URL when there are nosearchParams
Changelog: [Feature] Prevent adding runModule & modulesOnly search params when processing bundle async paths for production bundles
Test plan
- [x] - existing tests pass
- [x] - new test passes
hey @vzaidman 👋, I've taken that from here: https://github.com/facebook/metro/blob/6f0093c4cdc597dbbde8b22b159b9cb0b7912865/packages/metro/src/DeltaBundler/Serializers/helpers/tests/js-test.js#L179
(although that might have been an over-interpretation on my side 😅)
@vzaidman is there anything we can do about this PR to move forward with it? 🙏
@jbroma sorry about the delay with this.
Could you remind me - is there a functional difference intended here (eg MF using different defaults, so we want to leave these params implicit), or is this purely tidying up?
@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@jbroma sorry about the delay with this.
Could you remind me - is there a functional difference intended here (eg MF using different defaults, so we want to leave these params implicit), or is this purely tidying up?
No worries, I feel like we got a little bit sidetracked and left those PRs be for the past month or so, so sorry about that too 😅
So this PR is mostly about tidying up the bundle output in production mode. This PR is completely optional for Module Federation but it cleans up the split module IDs by removing the query params from them.
This should not be breaking in any way because if the query params in question are already there they will be replaced just like before - and that seems to be the case, even for nested lazily imported bundles.
@jbroma I just had a closer look at this and notice the params we set here are the opposite of the defaults:
https://github.com/facebook/metro/blob/4f2110412e3c4cbe6feb4f4cad27085edf7db1b0/packages/metro/src/lib/parseOptionsFromUrl.js#L72
https://github.com/facebook/metro/blob/4f2110412e3c4cbe6feb4f4cad27085edf7db1b0/packages/metro/src/lib/parseOptionsFromUrl.js#L75
So these params are overrides to Metro defaults, and they're typically necessary when loading additional module definitions into an existing bundle from the Metro server (you don't need the bundle boilerplate hence modulesOnly: true, and it's up to the caller to evaluate hence runModule: false).
The effect of this PR is that if you don't explicitly specify modulesOnly=false (the default) when loading the entry bundle from Metro, then modulesOnly won't be overridden to true for lazy imports - which potentially breaks lazy bundling from the Metro server.
For your use case, I guess you're loading prebuilt bundles in prod and so the query params are just noise. But I think the more correct approach for that is to manipulate the URL only when loading from the dev server, rather than basing it on whether the particular params are present.
Ah, that makes sense, haven't thought of that implicit edge case. I'll rework this to a dev-only feature then 👍 Thanks for investigating this deeply 🙏
@robhogan I've aligned the PR to prevent overriding async bundle paths in prod only 👍
@robhogan has imported this pull request. If you are a Meta employee, you can view this in D77455249.