feat(esm): leverage loaders when resolving subsequent loaders #43772

arcanis · 2022-07-11T11:28:28Z

“Notable change” summary by @GeoffreyBooth:

Loaders now apply to subsequent loaders, for example: --experimental-loader ts-node --experimental-loader loader-written-in-typescript.

Original introduction by @arcanis:

This PR is a follow-up to the Ambient Loaders proposal. Since in the last meetings we weren’t too sure whether “ambient loaders” shouldn’t just be the default loaders behaviour, I implemented it this way (but can change it later if needed).

I left a few assets to quickly try the system; without this PR, the second --loader would fail because xxx/* wouldn’t be resolvable:

cd dev_fixtures_do_not_merge
../node –loader ./loader-a.mjs –loader xxx/loader-b.mjs ./index.mjs

cc @nodejs/loaders @cspotcode

lib/internal/process/esm_loader.js

dev_fixtures_do_not_merge/index.mjs

GeoffreyBooth · 2022-07-19T22:42:33Z

lib/internal/process/esm_loader.js

+  for (let i = 0; i < customLoaders.length; i++) {
+    const customLoader = customLoaders[i];
+
+    // Importation must be handled by internal loader to avoid poluting userland
+    const keyedExportsSublist = await internalEsmLoader.import(
+      [customLoader],
+      pathToFileURL(cwd).href,
+      ObjectCreate(null),
+    );
+
+    await internalEsmLoader.addCustomLoaders(keyedExportsSublist);
+    ArrayPrototypePushApply(keyedExportsList, keyedExportsSublist);
+  }


A) How is implementing “loaders applying to subsequent loaders” so simple, and B) does this break any existing tests?

Looking at this code, there’s no way I would’ve guessed that this was using existing loaders as part of resolving and loading subsequent loaders. Could you add a comment (or several) explaining that that’s what’s happening here, and how?

My naive thinking is that since loader resolution goes through a loader pipeline (even if it doesn't initially contain any loader but the default one), all we need is to progressively add the loaders to it so that they are used by subsequent resolutions.

~~A few tests don't seem to pass yet, I'm searching why.~~

lib/internal/process/esm_loader.js

JakobJingleheimer

I'm a bit confused by the approach here, so I reviewed only the first part of this PR. I would expect something like this to suffice:

Import ambient loaders (via internal ESMLoader)
Add ambient loaders to internal ESMLoader
Import lay loaders (via internal ESMLoader, thus affected by ambient loaders added above)
Add ambient loaders to public ESMLoader
Add lay loaders to public ESMLoader

main...JakobJingleheimer:node:feat/add-esm-ambient-loaders

Could you help me understand why the extra complexity is necessary?

EDIT: If the above is sufficient, I'd be happy to pick this up (seems pretty quick to knock out), but also don't want to step on your toes.

lib/internal/process/esm_loader.js

lib/internal/modules/esm/loader.js

lib/internal/process/esm_loader.js

JakobJingleheimer · 2022-08-08T07:43:58Z

lib/internal/process/esm_loader.js

+    const keyedExportsSublist = await internalEsmLoader.import(
+      [customLoader],
+      pathToFileURL(cwd).href,
+      ObjectCreate(null),
+    );


Why loop over customLoaders just to feed each one as a wrapped list to ESMLoader::import() (which already supports multiple specifiers)?

I would expect simply:

const keyedExportsSublist = internalESMLoader(customLoaders); internalESMLoader.addCustomLoaders(keyedExportsSublist); ArrayPrototypePushApply(loaders, keyedExportsSublist);

This diff makes it so that we load loaders one at a time (because each one must be used to resolve/loader the subsequent ones).

Why the wrapper: without it, import makes an early return which shortcuts the pairing.

arcanis · 2022-08-08T08:30:48Z

Could you help me understand why the extra complexity is necessary?

Your implementation seems to split the loaders in two, but doesn't entirely answer the requirement to "leverage loaders when resolving subsequent loaders". For instance, with your diff, only the following would work:

node --ambient-loader pnp --loader ts-node

And the following wouldn't work:

node --ambient-loader pnp --ambient-loader ts-node

In other words, --ambient-loader would de-facto be restricted to a single loader, because others wouldn't be resolvable (just like vendored loaders cannot be resolved without ambient loaders).

JakobJingleheimer · 2022-08-08T08:41:34Z

Your implementation seems to split the loaders in two, but doesn't entirely answer the requirement to "leverage loaders when resolving subsequent loaders".

Ahhh, yes: ambient loaders need to be imported and also added to the list of hooks before the next is imported (so the previous hook runs during the subsequent's import)

🤔

lib/internal/process/esm_loader.js

josuevalrob · 2022-09-27T08:01:59Z

hello :)
what is the status here?

GeoffreyBooth · 2022-09-27T17:13:12Z

The plan is to land this once it’s ready and after #44710.

arcanis · 2022-11-13T10:14:50Z

From the last loaders meeting notes:

@arcanis will rebase #43772, we’ll re-review it, and assuming it’s fine we’ll land it, and it can be released in 19.x. I’ll mark it as blocked from release on 18 and below until the off-thread PR lands or Jan. 1, 2023, whichever comes first, to avoid unnecessary churn for the majority of users.

GeoffreyBooth · 2022-11-13T18:25:21Z

@arcanis Can you please rebase this on main and take out the merge commit? All Node PRs need to be “clean” like that so that the history can be bisected. You can squash your commits together first if that makes the rebase easier.

JakobJingleheimer

I'm almost in disbelief how simple this is 🙌 Seeing that, I don't anticipate any clashes with off-threading.

I think the ESM doc just needs to be updated and this is good to go.

EDIT: And perhaps some code docs about the new interactive behaviour between loaders?

JakobJingleheimer · 2022-11-19T12:07:56Z

lib/internal/modules/esm/loader.js

@@ -306,7 +306,7 @@ class ESMLoader {

  /**
   * Collect custom/user-defined hook(s). After all hooks have been collected,
-   * calls global preload hook(s).
+   * the global preload hook(s) must be called.


Maybe a bit of explanation as to why this can't happen automatically? (since it previously did)

Cf this thread:

Because otherwise, since addCustomLoaders is now called multiple times (once for each loader), it would lead to preload being called multiple times, which in turn would execute the global preload hook multiple times, which would crash (you can try reverting this change; a test crashes).

Sure, something like that'll do 🙂 (I'm suggesting adding a code comment)

lib/internal/process/esm_loader.js

test/es-module/test-esm-loader-chaining.mjs

test/fixtures/es-module-loaders/loader-load-foo-or-42.mjs

test/fixtures/es-module-loaders/loader-resolve-strip-xxx.mjs

test/fixtures/es-module-loaders/loader-resolve-strip-yyy.mjs

test/es-module/test-esm-loader-chaining.mjs

lizthegrey · 2023-03-31T00:57:54Z

Would you please backport this to v18?

+1 to backport request; this functionality is necessary for use of Yarn PNP alongside https://github.com/DataDog/import-in-the-middle as per yarnpkg/berry#4044

targos · 2023-03-31T06:30:34Z

@nodejs/loaders We should probably prepare a single backport PR with all recent ESM-loader changes.

GeoffreyBooth · 2023-04-02T00:25:25Z

And/or this one can go on its own. There haven’t been too many ESM loader PRs lately, other than some test improvements. This was @arcanis’s PR, @arcanis do you mind doing the backports?

bencmbrook · 2023-05-12T02:08:29Z

Still planning on backporting to 18.x @GeoffreyBooth / @arcanis ?

bencmbrook · 2023-05-16T23:12:55Z

Splitting the backport question into an Issue here #48042

lizthegrey · 2023-07-20T00:23:14Z

This was missed for backport into 18.17.0 somehow.

PR-URL: #43772 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

lizthegrey · 2023-11-12T20:25:01Z

Thanks for backporting y'all.

Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908 * (SEMVER-MINOR) upgrade npm to 10.0.0 (npm team) #49423 doc: * add new TSC members (Michael Dawson) #48841 * move and rename loaders section (Geoffrey Booth) #49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) #50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869 * unflag import.meta.resolve (Guy Bedford) #49028 * move hook execution to separate thread (Jacob Smith) #44710 * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) #49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141 wasi: * (SEMVER-MINOR) updates required for latest uvwasi version (Michael Dawson) #49908 PR-URL: TODO

Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) #50531 doc: * move and rename loaders section (Geoffrey Booth) #49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) #50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869 * unflag import.meta.resolve (Guy Bedford) #49028 * move hook execution to separate thread (Jacob Smith) #44710 * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) #49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141 PR-URL: #50953

PR-URL: nodejs/node#43772 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs/node#49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) nodejs/node#50531 doc: * move and rename loaders section (Geoffrey Booth) nodejs/node#49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs/node#49869 * unflag import.meta.resolve (Guy Bedford) nodejs/node#49028 * move hook execution to separate thread (Jacob Smith) nodejs/node#44710 * leverage loaders when resolving subsequent loaders (Maël Nison) nodejs/node#43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) nodejs/node#46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) nodejs/node#44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) nodejs/node#45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) nodejs/node#49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs/node#49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs/node#49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs/node#48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs/node#48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs/node#47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs/node#49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs/node#45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50141 PR-URL: nodejs/node#50953

PR-URL: nodejs/node#43772 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs/node#49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) nodejs/node#50531 doc: * move and rename loaders section (Geoffrey Booth) nodejs/node#49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs/node#49869 * unflag import.meta.resolve (Guy Bedford) nodejs/node#49028 * move hook execution to separate thread (Jacob Smith) nodejs/node#44710 * leverage loaders when resolving subsequent loaders (Maël Nison) nodejs/node#43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) nodejs/node#46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) nodejs/node#44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) nodejs/node#45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) nodejs/node#49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs/node#49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs/node#49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs/node#48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs/node#48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs/node#47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs/node#49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs/node#45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50141 PR-URL: nodejs/node#50953

nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Jul 11, 2022

arcanis force-pushed the mael/loaders-subsequent-resolution branch from 9044b9c to b234023 Compare July 11, 2022 11:41

aduh95 reviewed Jul 11, 2022

View reviewed changes

lib/internal/process/esm_loader.js Outdated Show resolved Hide resolved

lib/internal/process/esm_loader.js Outdated Show resolved Hide resolved

GeoffreyBooth reviewed Jul 11, 2022

View reviewed changes

dev_fixtures_do_not_merge/index.mjs Outdated Show resolved Hide resolved

arcanis marked this pull request as ready for review July 19, 2022 08:49

GeoffreyBooth reviewed Jul 19, 2022

View reviewed changes

arcanis mentioned this pull request Jul 31, 2022

[Bug?]: chain loader yarnpkg/berry#4694

Closed

1 task

loynoir mentioned this pull request Jul 31, 2022

[Bug?]: chain loader privatenumber/tsx#72

Closed

1 task

sheremet-va mentioned this pull request Aug 2, 2022

[0.20.x] vitest no longer works with yarn pnp vitest-dev/vitest#1758

Closed

6 tasks

GeoffreyBooth mentioned this pull request Aug 4, 2022

Update roadmap nodejs/loaders#108

Merged

shrujalshah28 reviewed Aug 5, 2022

View reviewed changes

lib/internal/process/esm_loader.js Outdated Show resolved Hide resolved

JakobJingleheimer reviewed Aug 8, 2022

View reviewed changes

targos reviewed Aug 8, 2022

View reviewed changes

lib/internal/process/esm_loader.js Outdated Show resolved Hide resolved

arcanis mentioned this pull request Aug 13, 2022

Node.js Loaders Team Meeting 2022-08-16 nodejs/loaders#109

Closed

GeoffreyBooth mentioned this pull request Sep 19, 2022

test: add test for Module._stat #44713

Merged

jasnell approved these changes Oct 2, 2022

View reviewed changes

loynoir mentioned this pull request Oct 5, 2022

esm, loader: move to own thread #43658

Closed

1 task

arcanis mentioned this pull request Nov 4, 2022

Node.js Loaders Team Meeting 2022-11-08 nodejs/loaders#117

Closed

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

arcanis force-pushed the mael/loaders-subsequent-resolution branch 2 times, most recently from 70f2987 to 7f30aae Compare November 18, 2022 14:21

JakobJingleheimer approved these changes Nov 19, 2022

View reviewed changes

aduh95 reviewed Nov 19, 2022

View reviewed changes

test/es-module/test-esm-loader-chaining.mjs Outdated Show resolved Hide resolved

rtritto mentioned this pull request Mar 18, 2023

[Node 19 + ESM + PNP] ERR_MODULE_NOT_FOUND TypeStrong/ts-node#1972

Closed

JamieDanielson mentioned this pull request Apr 4, 2023

feat: ESM support for instrumentation open-telemetry/opentelemetry-js#3698

Merged

8 tasks

clemyan mentioned this pull request Apr 12, 2023

ESM loader chaining while loading loaders regression in 19.8.0 #47526

Closed

joyeecheung mentioned this pull request Apr 18, 2023

esm: do not lazy instantiate loaders to avoid infinite loop #47599

Closed

merceyz mentioned this pull request Apr 19, 2023

loader: use default loader as cascaded loader in the in loader worker #47620

Merged

bencmbrook mentioned this pull request May 16, 2023

Backport ESM loader changes to 18.x #48042

Closed

aduh95 mentioned this pull request Jun 2, 2023

esm: remove support for arrays in import internal method #48296

Merged

petersalas mentioned this pull request Jun 15, 2023

Switch to yarn PnP fixie-ai/ai-jsx#48

Merged

izaakschroeder mentioned this pull request Jun 27, 2023

esm: unflag Module.register and allow nested loader import() #48559

Merged

targos pushed a commit that referenced this pull request Nov 10, 2023

esm: leverage loaders when resolving subsequent loaders

490b598

PR-URL: #43772 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

christian-bromann mentioned this pull request Dec 7, 2023

[🐛 Bug]: Cannot use import statement outside a module with Node 18.19.0 webdriverio/webdriverio#11825

Closed

3 tasks

voltrevo mentioned this pull request Dec 12, 2023

Fix inpage demo in node 18.19+ getwax/wax#174

Closed

gthb mentioned this pull request Dec 12, 2023

Line numbers in stack traces are wrong in Node v20 TypeStrong/ts-node#2053

Open

garrettjstevens mentioned this pull request Dec 14, 2023

Oclif does not work with Yarn 4 PnP oclif/oclif#1222

Open

caugner mentioned this pull request Apr 8, 2024

fix(scripts): specify loader via NODE_OPTIONS mdn/yari#10865

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(esm): leverage loaders when resolving subsequent loaders #43772

feat(esm): leverage loaders when resolving subsequent loaders #43772

arcanis commented Jul 11, 2022 •

edited by GeoffreyBooth

Loading

GeoffreyBooth Jul 19, 2022

arcanis Jul 21, 2022 •

edited

Loading

JakobJingleheimer left a comment •

edited

Loading

JakobJingleheimer Aug 8, 2022

arcanis Aug 8, 2022

arcanis commented Aug 8, 2022

JakobJingleheimer commented Aug 8, 2022

josuevalrob commented Sep 27, 2022

GeoffreyBooth commented Sep 27, 2022 •

edited

Loading

arcanis commented Nov 13, 2022

GeoffreyBooth commented Nov 13, 2022

JakobJingleheimer left a comment •

edited

Loading

JakobJingleheimer Nov 19, 2022

arcanis Nov 23, 2022

JakobJingleheimer Nov 25, 2022

lizthegrey commented Mar 31, 2023

targos commented Mar 31, 2023

GeoffreyBooth commented Apr 2, 2023

bencmbrook commented May 12, 2023 •

edited

Loading

bencmbrook commented May 16, 2023

lizthegrey commented Jul 20, 2023

lizthegrey commented Nov 12, 2023

feat(esm): leverage loaders when resolving subsequent loaders #43772

feat(esm): leverage loaders when resolving subsequent loaders #43772

Conversation

arcanis commented Jul 11, 2022 • edited by GeoffreyBooth Loading

GeoffreyBooth Jul 19, 2022

Choose a reason for hiding this comment

arcanis Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

JakobJingleheimer left a comment • edited Loading

Choose a reason for hiding this comment

JakobJingleheimer Aug 8, 2022

Choose a reason for hiding this comment

arcanis Aug 8, 2022

Choose a reason for hiding this comment

arcanis commented Aug 8, 2022

JakobJingleheimer commented Aug 8, 2022

josuevalrob commented Sep 27, 2022

GeoffreyBooth commented Sep 27, 2022 • edited Loading

arcanis commented Nov 13, 2022

GeoffreyBooth commented Nov 13, 2022

JakobJingleheimer left a comment • edited Loading

Choose a reason for hiding this comment

JakobJingleheimer Nov 19, 2022

Choose a reason for hiding this comment

arcanis Nov 23, 2022

Choose a reason for hiding this comment

JakobJingleheimer Nov 25, 2022

Choose a reason for hiding this comment

lizthegrey commented Mar 31, 2023

targos commented Mar 31, 2023

GeoffreyBooth commented Apr 2, 2023

bencmbrook commented May 12, 2023 • edited Loading

bencmbrook commented May 16, 2023

lizthegrey commented Jul 20, 2023

lizthegrey commented Nov 12, 2023

arcanis commented Jul 11, 2022 •

edited by GeoffreyBooth

Loading

arcanis Jul 21, 2022 •

edited

Loading

JakobJingleheimer left a comment •

edited

Loading

GeoffreyBooth commented Sep 27, 2022 •

edited

Loading

JakobJingleheimer left a comment •

edited

Loading

bencmbrook commented May 12, 2023 •

edited

Loading