esm: refactor responseURL handling by guybedford · Pull Request #43164 · nodejs/node

guybedford · 2022-05-21T00:56:59Z

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 · 2022-05-21T00:57:03Z

Review requested:

@nodejs/modules
@nodejs/vm

nodejs-github-bot · 2022-05-23T03:12:47Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44135/

nodejs-github-bot · 2022-05-27T21:17:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44200/

JakobJingleheimer

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)

This reverts commit a2923a9.

nodejs-github-bot · 2022-06-03T02:29:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44324/

nodejs-github-bot · 2022-06-03T20:03:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/44344/

PR-URL: #43164 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>

guybedford · 2022-06-03T21:29:54Z

Landed in dfa896f.

PR-URL: #43164 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>

danielleadams · 2022-06-13T00:23:38Z

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 · 2022-06-13T00:37:54Z

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

PR-URL: #43164 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>

PR-URL: nodejs/node#43164 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>

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 May 21, 2022

ljharb reviewed May 21, 2022
View reviewed changes

Comment threadlib/internal/modules/esm/load.js Outdated

guybedford force-pushed the resolved-url branch from 9e79269 to 99113e7Compare May 21, 2022 07:14

aduh95 reviewed May 21, 2022
View reviewed changes

Comment threadlib/internal/modules/esm/load.js Outdated
Comment threadlib/internal/modules/esm/load.js Outdated
Comment threadlib/internal/modules/esm/load.js Outdated

aduh95 reviewed May 21, 2022
View reviewed changes

Comment threadtest/es-module/test-esm-loader-http-imports.mjs Outdated

aduh95 reviewed May 21, 2022
View reviewed changes

Comment threadlib/internal/modules/esm/load.js Outdated

aduh95 approved these changes May 22, 2022
View reviewed changes

aduh95 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-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 22, 2022

This comment was marked as outdated.
Sign in to view

This was referenced May 23, 2022
CI Reliability 2022-05-23 nodejs/reliability#286
Open
CI Reliability 2022-05-24 nodejs/reliability#287
Open

JungMinu approved these changes May 24, 2022
View reviewed changes

This was referenced May 25, 2022
CI Reliability 2022-05-25 nodejs/reliability#288
Open
CI Reliability 2022-05-26 nodejs/reliability#289
Open

esm: refactor responseURL handling
b884fab

guybedford force-pushed the resolved-url branch from da7c4d4 to b884fabCompare May 27, 2022 20:44

fix: adjust responseURL error
580b8b7

This was referenced May 28, 2022
CI Reliability 2022-05-28 nodejs/reliability#291
Open
CI Reliability 2022-05-29 nodejs/reliability#292
Open

JakobJingleheimer approved these changes May 29, 2022
View reviewed changes

JakobJingleheimer added loaders Issues and PRs related to ES module loaders esm Issues and PRs related to the ECMAScript Modules implementation. labels May 29, 2022

JakobJingleheimer mentioned this pull request May 29, 2022
esm custom loader: multiple calls to nextHook within the same loader result in different nextHooks #43238
Closed

parentURL, responseURL -> parentUrl, responseUrl
a2923a9

ljharb reviewed Jun 1, 2022
View reviewed changes

Comment threaddoc/api/esm.md Outdated

github-actionsBot mentioned this pull request Jun 2, 2022
CI Reliability 2022-06-02 nodejs/reliability#296
Open
19 tasks

JakobJingleheimer mentioned this pull request Jun 2, 2022
esm: fix chain advances when loader calls next<HookName> multiple times #43303
Merged

github-actionsBot mentioned this pull request Jun 3, 2022
CI Reliability 2022-06-03 nodejs/reliability#297
Open
19 tasks

Revert "parentURL, responseURL -> parentUrl, responseUrl"
1063036
This reverts commit a2923a9.

guybedford added a commit that referenced this pull request Jun 3, 2022
esm: refactor responseURL handling
dfa896f
PR-URL: #43164 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>

guybedford closed this Jun 3, 2022

guybedford deleted the resolved-url branch June 3, 2022 21:29

github-actionsBot mentioned this pull request Jun 4, 2022
CI Reliability 2022-06-04 nodejs/reliability#298
Open
21 tasks

danielleadams pushed a commit that referenced this pull request Jun 11, 2022
esm: refactor responseURL handling
86bafe5
PR-URL: #43164 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>

danielleadams mentioned this pull request Jun 11, 2022
v18.4.0 proposal #43385
Merged

danielleadams added the backport-blocked-v18.x label Jun 13, 2022

JakobJingleheimer mentioned this pull request Jun 15, 2022
esm: convert resolve hook to synchronous #43363
Merged

JakobJingleheimer removed the backport-blocked-v18.x label Jun 18, 2022

targos pushed a commit that referenced this pull request Jul 12, 2022
esm: refactor responseURL handling
50d64ed
PR-URL: #43164 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>

targos mentioned this pull request Jul 12, 2022
v18.6.0 release proposal #43789
Merged

targos pushed a commit that referenced this pull request Jul 19, 2022
esm: refactor responseURL handling
1bd62be
PR-URL: #43164 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>

targos pushed a commit that referenced this pull request Jul 31, 2022
esm: refactor responseURL handling
eac4e20
PR-URL: #43164 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me>

targos mentioned this pull request Aug 3, 2022
v16.17.0 release proposal #44098
Merged

andrew0 mentioned this pull request Dec 2, 2023
retain query string in resolved paths tapjs/tsimp#5
Closed

Uh oh!

Uh oh!

Conversation

guybedford commented May 21, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented May 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented May 23, 2022

Uh oh!

nodejs-github-bot commented May 27, 2022

Uh oh!

JakobJingleheimer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 3, 2022

Uh oh!

nodejs-github-bot commented Jun 3, 2022

Uh oh!

guybedford commented Jun 3, 2022

Uh oh!

danielleadams commented Jun 13, 2022

Uh oh!

guybedford commented Jun 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

guybedford commented May 21, 2022•
edited
Loading

JakobJingleheimer left a comment •
edited
Loading