Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Jul 11, 2022

“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

@nodejs-github-bot 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 arcanis force-pushed the mael/loaders-subsequent-resolution branch from 9044b9c to b234023 Compare July 11, 2022 11:41
lib/internal/process/esm_loader.js Outdated Show resolved Hide resolved
lib/internal/process/esm_loader.js Outdated Show resolved Hide resolved
@arcanis arcanis marked this pull request as ready for review July 19, 2022 08:49
Comment on lines 71 to 58
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);
}
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@arcanis arcanis Jul 21, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@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.

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:

  1. Import ambient loaders (via internal ESMLoader)
  2. Add ambient loaders to internal ESMLoader
  3. Import lay loaders (via internal ESMLoader, thus affected by ambient loaders added above)
  4. Add ambient loaders to public ESMLoader
  5. 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 Outdated Show resolved Hide resolved
lib/internal/modules/esm/loader.js Outdated Show resolved Hide resolved
lib/internal/process/esm_loader.js Outdated Show resolved Hide resolved
Comment on lines 77 to 54
const keyedExportsSublist = await internalEsmLoader.import(
[customLoader],
pathToFileURL(cwd).href,
ObjectCreate(null),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

arcanis commented Aug 8, 2022

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
Copy link
Contributor

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)

🤔

@josuevalrob
Copy link

hello :)
what is the status here?

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Sep 27, 2022

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

@arcanis
Copy link
Contributor Author

arcanis commented Nov 13, 2022

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
Copy link
Member

@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.

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

@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.

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?

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

lib/internal/process/esm_loader.js Outdated Show resolved Hide resolved
lib/internal/process/esm_loader.js Outdated Show resolved Hide resolved
lib/internal/process/esm_loader.js Outdated Show resolved Hide resolved
test/es-module/test-esm-loader-chaining.mjs Outdated Show resolved Hide resolved
test/fixtures/es-module-loaders/loader-load-foo-or-42.mjs Outdated Show resolved Hide resolved
@lizthegrey
Copy link

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
Copy link
Member

targos commented Mar 31, 2023

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

@GeoffreyBooth
Copy link
Member

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
Copy link

bencmbrook commented May 12, 2023

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

@bencmbrook
Copy link

Splitting the backport question into an Issue here #48042

@lizthegrey
Copy link

This was missed for backport into 18.17.0 somehow.

targos pushed a commit that referenced this pull request Nov 10, 2023
PR-URL: #43772
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
@lizthegrey
Copy link

Thanks for backporting y'all.

targos added a commit that referenced this pull request Nov 27, 2023
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
targos added a commit that referenced this pull request Nov 28, 2023
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
targos added a commit that referenced this pull request Nov 29, 2023
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
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#43772
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#43772
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders notable-change PRs with changes that should be highlighted in changelogs. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.