Skip to content

Conversation

@bmeck
Copy link
Member

@bmeck bmeck commented Feb 25, 2022

Per the PR text this kind of explains what is going on:

  /**
   * ...
   * 
   * In WHATWG HTTP spec for ESM the cache key is the non-I/O bound
   * synchronous resolution using only string operations
   *   ~= resolveImportMap(new URL(specifier, importerHREF))
   * 
   * The url used for subsequent resolution is the response URL after
   * all redirects have been resolved.
   * 
   * https://example.com/foo redirecting to https://example.com/bar
   * would have a cache key of https://example.com/foo and baseURL
   * of https://example.com/bar
   * 
   * ...
   */

Likely this should be a bigger refactor because callbackMap is becoming spaghetti but that should/can be punted for now.

This is 1/2 of the issues we have seen with HTTPS CDNs for ESM and is a partial fix for #42098 .

I would note that the URL mentioned in the corresponding issue continues to fail even with this and #42119 because it redirects to an absolute URL with a 404:

302 https://unpkg.com/uuid?module ->
200 https://unpkg.com/uuid@8.3.2/dist/esm-node/index.js?module -> (this is where the PR fixes apply in both PRs)
200 https://unpkg.com/uuid@8.3.2/dist/esm-node/rng.js?module ->
404 https://unpkg.com/crypto@latest?module

Before this PR the base URL used for resolving ./rng.js was incorrectly using the cache key of the module https://unpkg.com/uuid?module. Per WHATWG this should actually be changed to https://unpkg.com/uuid@8.3.2/dist/esm-node/index.js?module

Set base URL to referencing script's base URL.

And WHATWG:

It is intentional that the module map is keyed by the request URL, whereas the base URL for the module script is set to the response URL. The former is used to deduplicate fetches, while the latter is used for URL resolution.

After both PRs the behavior in Node matches the browser: https://jsfiddle.net/n8cwL4bm/

@bmeck bmeck added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Feb 25, 2022
@bmeck bmeck requested a review from guybedford February 25, 2022 16:43
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Feb 25, 2022
@bmeck
Copy link
Member Author

bmeck commented Feb 26, 2022

this PR depends on #42119

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Looks good, if we move to multiple loaders in the same context we might have to reconsider the approach though.

Agreed it would be nice to clean up the callbackMap for a better approach at some point.

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

Thanks for cranking this out so fast! 🙌 This was a bit of a doozy to investigate and noodle.

@nodejs-github-bot
Copy link
Collaborator

@bmeck
Copy link
Member Author

bmeck commented Mar 2, 2022

had to rebase due to test failure, going to land in after CI and a small wait

@bmeck bmeck added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2022
@bmeck bmeck added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Mar 3, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@bmeck
Copy link
Member Author

bmeck commented Mar 4, 2022

CI tests look to be flaky per @nodejs/reliability, running last CI then merging if no other tests fail

@bmeck
Copy link
Member Author

bmeck commented Mar 4, 2022

Landed in acc11c9

@danielleadams

This comment was marked as outdated.

@danielleadams
Copy link
Contributor

Waiting on backport: #42726

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory. loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants