Skip to content

Conversation

@guybedford
Copy link
Contributor

@guybedfordguybedford commented May 21, 2022

Refactors responseURL handling solving the same issue as #43130 but with the architecture changes to responseURL as discussed in that PR by treating the module wrap, translators, parentUrl in resolution and providers URLs as the responseURL, as opposed to the module map URL, the resolved URL and the first argument to load as the initial URL. This is consistent with web specifications and gives the correct relative resolution, import.meta.url etc without further function calls being necessary.

In addition this exposes the responseURL return value to the load hook, which is optional. This is landing as treating that as an undocumented loader API for now, since it's generally not an encouraged pattern but is needed by the core loader piping. Whether it should be a public API is a question for the loaders design more generally.

@nodejs/modules @nodejs/loaders

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules
  • @nodejs/vm

@nodejs-github-botnodejs-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 May 21, 2022
@aduh95aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 22, 2022
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@JakobJingleheimerJakobJingleheimer left a comment

Choose a reason for hiding this comment

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

Huzzah! Thank you for handling this. A couple nits and a possible naming thing, nothing big 😁

I see you removed the fetch cache check: could you add a test case to test/es-module/test-esm-loader-http-imports.mjs to ensure only the 1 fetch happens? (I can do if you prefer)

@JakobJingleheimerJakobJingleheimer added loaders Issues and PRs related to ES module loaders esm Issues and PRs related to the ECMAScript Modules implementation. labels May 29, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

guybedford added a commit that referenced this pull request Jun 3, 2022
PR-URL: #43164 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
@guybedford
Copy link
ContributorAuthor

Landed in dfa896f.

@guybedfordguybedford deleted the resolved-url branch June 3, 2022 21:29
italojs pushed a commit to italojs/node that referenced this pull request Jun 6, 2022
PR-URL: nodejs#43164 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 11, 2022
PR-URL: #43164 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Jun 11, 2022
danielleadams pushed a commit that referenced this pull request Jun 13, 2022
PR-URL: #43164 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
@danielleadams
Copy link
Contributor

This didn't land cleanly in v18.x (maybe related to #42623 or the work mentioned here #43385 (comment)), so adding a backport-blocked label.

@guybedford
Copy link
ContributorAuthor

@danielleadams yes, this should land after #43130, which in turn should land after #42623.

danielleadams pushed a commit that referenced this pull request Jun 13, 2022
PR-URL: #43164 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43164 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
@targostargos mentioned this pull request Jul 12, 2022
targos pushed a commit that referenced this pull request Jul 19, 2022
PR-URL: #43164 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43164 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
@targostargos mentioned this pull request Aug 3, 2022
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43164 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.esmIssues and PRs related to the ECMAScript Modules implementation.lib / srcIssues and PRs related to general changes in the lib or src directory.loadersIssues and PRs related to ES module loadersneeds-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@guybedford@nodejs-github-bot@danielleadams@ljharb@bmeck@GeoffreyBooth@JakobJingleheimer@JungMinu@aduh95