fix(node-loader): short-circuit http(s) resolves and handle empty parentURL - #78
Open
beldevem wants to merge 1 commit into
Open
Conversation
…entURL - resolve() now short-circuits for absolute http(s) URLs (mapped via the import map, a sibling-chunk relative specifier, or an already-absolute specifier), mirroring what load() already does. Without this, Node's nextResolve() falls through to package.json-scope resolution and throws ERR_INVALID_URL for any remote whose bundle has a chunk importing another chunk or a scoped shared dependency. - context.parentURL can arrive as an empty string (not just missing) under module.register()'s worker-thread hook execution, which broke both relative sibling-chunk resolution and import-map scope matching (it silently fell back to the local cwd file:// base). load() now records the last remote base URL it fetched, and resolve() falls back to it when parentURL is empty. - load() now also returns responseURL so relative imports from within a loaded remote module have a correct base even when parentURL propagates correctly. Fixes native-federation#77
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #77
Problem
resolve()insrc/scripts/node-loader.tsdidn't short-circuit for absolutehttp(s)URLs the wayload()already does, causing two real-world failures when a native-federation host loads a remote over http(s) in a Node SSR process (module.register()-based loader hooks):./chunk-abc.jsimported fromhttps://remote-host/remote/entry.js) isn't in the import map, soresolve()fell through tonextResolve()which only understandsfile:/data:/node:specifiers and threwERR_INVALID_URL.context.parentURLcan arrive as an empty string (notundefined) undermodule.register()'s worker-thread hook execution.resolveSpecifier()'sparentURL ? ... : baseURLcheck treated the empty string as falsy and silently fell back to the localbaseURLinstead of the remote's base, breaking both relative sibling-chunk resolution and import-map scope matching for that remote.Fix
resolve()now short-circuits for absolutehttp(s)URLs whether mapped via the import map, resolved as a relative sibling-chunk specifier, or already absolute mirroring the short-circuitload()already does.load()now records the last remote base URL it fetched (lastRemoteBaseURL);resolve()falls back to it whencontext.parentURLis empty, so relative sibling-chunk imports and scope matching keep working even when the parent URL is lost.load()now also returnsresponseURLfor http(s) modules.Tests
Added coverage in
node-loader.spec.tsfor the previously-untested scenarios: a mappedhttp(s)specifier short-circuiting instead of hittingnextResolve, a relative sibling-chunk specifier resolving via the URL algorithm, and the empty-parentURLfallback to the last loaded remote base. Updated the two existingload()tests to account for the newresponseURLfield. All 25 tests pass;node-loader.tsitself has zero TypeScript/lint errors.Related: native-federation/angular-adapter#132 (filed against the wrong repo before the actual bug source was traced here).