node
node copied to clipboard
nodejs
feat(esm): leverage loaders when resolving subsequent loaders
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
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).
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)
🤔
The plan is to land this once it’s ready and after https://github.com/nodejs/node/pull/44710.
From the last loaders meeting notes:
@arcanis will rebase https://github.com/nodejs/node/pull/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.
@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 I’d love to land this, if you have time to get it passing CI.
Thanks for reminding me, it should be all good now! The test was failing because of a typo made during the last merge conflict resolution.
Adding don't land label for now because I think the other loaders PR should be ready soon and it would be best to release them together.
Commit Queue failed
- Loading data for nodejs/node/pull/43772 ✔ Done loading data for nodejs/node/pull/43772 ----------------------------------- PR info ------------------------------------ Title feat(esm): leverage loaders when resolving subsequent loaders (#43772) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch arcanis:mael/loaders-subsequent-resolution -> nodejs:main Labels process, esm, author ready, loaders, commit-queue-squash, dont-land-on-v19.x Commits 1 - esm: leverage loaders when resolving subsequent loaders Committers 1 - Maël Nisonhttps://github.com/nodejs/node/actions/runs/3718340380PR-URL: https://github.com/nodejs/node/pull/43772 Reviewed-By: James M Snell Reviewed-By: Jacob Smith Reviewed-By: Geoffrey Booth ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/43772 Reviewed-By: James M Snell Reviewed-By: Jacob Smith Reviewed-By: Geoffrey Booth -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - esm: leverage loaders when resolving subsequent loaders ℹ This PR was created on Mon, 11 Jul 2022 11:28:28 GMT ✔ Approvals: 3 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/43772#pullrequestreview-1195007294 ✔ - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/43772#pullrequestreview-1187065521 ✔ - Geoffrey Booth (@GeoffreyBooth) (TSC): https://github.com/nodejs/node/pull/43772#pullrequestreview-1193676228 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2022-12-16T23:36:33Z: https://ci.nodejs.org/job/node-test-pull-request/48537/ - Querying data for job/node-test-pull-request/48537/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu
So excited for this. Any indication of whether this will make it into LTS / 18.x?
So excited for this. Any indication of whether this will make it into LTS / 18.x?
Yes, it should.
Adding don't land label for now because I think the other loaders PR should be ready soon and it would be best to release them together. https://github.com/nodejs/node/pull/43772#issuecomment-1356014538
Adding dont-land-on-v19.x goes against the decision from the loaders meeting though https://github.com/nodejs/loaders/blob/4a32bb8a0f0e0da4a330c0b6426a2b162ff5cea2/doc/meetings/2022-11-08.md#notes.
Regardless, since we're now past 2023-01-01 shouldn't all the dont-land-on* labels be removed?
@nodejs/releasers per https://github.com/nodejs/loaders/issues/103#issuecomment-1397323186 can this please go out in the next release? Thanks.
Also this change is arguably notable, so I put a short release note in the top post.
cc @nodejs/loaders @arcanis
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 https://github.com/yarnpkg/berry/discussions/4044
@nodejs/loaders We should probably prepare a single backport PR with all recent ESM-loader changes.
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?
Splitting the backport question into an Issue here https://github.com/nodejs/node/issues/48042