esm: add chaining to loaders

[JakobJingleheimer]

Notable changes

• ESMLoader now provides more detailed error messages for hooks, including the specific file and hook from which the error was caused (ex the hook broke the chain; but not SyntaxError within the hook—which was already working). In order to facilitate this:
  • ESMLoader's internal collections of hooks have changed from hook fn[] → Array<{url: specifier, fn: hook }>
  • ESMLoader::import()'s return has changed from ModuleNamespace[] | ModuleNamespace → Array<{url: specifier, fn: hook }> | ModuleNamespace

TODOs:

• hook up next()s
• custom loaders FIFO → LIFO
• detect & throw on unsignalled short-circuit
• update existing hook output error messages to include newly available specifier
• update code docs
• update Error docs
• update ESM docs
• verify/fix node-land breakages caused the change to ESMLoader::import() (I think this will not be a breaking change to user-land as the functionality seen in user-land continues to be where ESMLoader::import() returns a single ModuleNamespace)
• update current tests
• add new tests for chaining

Trying it out

CLI

./node \ --loader=/PATH/TO/loaderA.mjs \ --loader=/PATH/TO/loaderB.mjs \ --input-type=module \ -e "import fs from 'node:fs'; console.log(2)"

loaderA.mjs

exportconstresolve=asyncfunctionresolveA(specifier,context,next,){console.log('resolveA running');return{shortCircuit: true,url: 'file:///foo/bar.js',};}exportconstload=asyncfunctionloadA(resolvedUrl,context,next,){console.log('loadA running');return{format: 'module',shortCircuit: true,source: 'export default 42',}}

loaderB.mjs

exportconstresolve=asyncfunctionresolveB(specifier,context,next,){console.log('resolveB running');returnnext(specifier);}exportconstload=asyncfunctionloadB(resolvedUrl,context,next,){console.log('loadB running');returnnext(resolvedUrl);}

42 should be printed to stdout when the hooks are properly chained; when disabled, the node:fs module should be printed to stdout.

[nodejs-github-bot]

Review requested:

• @nodejs/modules
• @nodejs/loaders

[JakobJingleheimer]

Hrrrm. I've got all the existing es-module tests updated except for es-module/test-esm-symlink-type.js: all of its cases are failing with the same TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension "" for […]. That error seems legit based on where it's coming from (ESMLoader doesn't support extensionless files), but the test seems to think it should be okay.

I didn't see anything related to symlinks when making my changes, so I'm not sure what's up 🤔

[bmeck]

It should have an extension after realpathing right?

…

On Wed, Apr 6, 2022, 4:57 PM Jacob Smith ***@***.***> wrote: Hrrrm. I've got all the existing es-module tests updated except for es-module/test-esm-symlink-type.js: all of its cases are failing with the same TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension "" for […]. That error seems legit based on where it's coming from (ESMLoader doesn't support extensionless files), but the test seems to think it should be okay. I didn't see anything related to symlinks when making my changes, so I'm not sure what's up 🤔 — Reply to this email directly, view it on GitHub <#42623 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJIY3J6WPKVHTYM66YD3VDYCEFANCNFSM5SVGTG3A> . You are receiving this because you are on a team that was mentioned.Message ID: ***@***.***>

[JakobJingleheimer]

I presume so based on your question. Do you know whereabouts I could check to confirm that is or isn't happening?

By the time it gets to ESMLoader::load() → defaultLoad() → getFormat() it (supposedly—I didn't print it) doesn't have an extension.

[GeoffreyBooth]

I haven’t reviewed the tests, but I’ve written more than enough notes so far 😄 Overall this is quite good; I agree with the general thrust and design. It shouldn’t take too much work to clean everything up and get this merged.

[JakobJingleheimer]

If possible, yes. I think #37468 made it into v16? (if so, it should be okay)

[targos]

Yes it did. I cherry-picked this change 👍🏻

[cspotcode]

Double-checking, is #43763 going to land in v16 as well? Otherwise it'll break projects.

[targos]

It wasn't going to be because by default commits are supposed to bake in a Current release before we consider them for LTS, but I just added the lts-watch label and will make sure it lands in v16 too.

[localjo]

It seems that this PR had breaking changes, but got merged into the 16.17.0 release. See the discussion in yarnpkg/berry#4778 for details.

[JakobJingleheimer]

@localjo it did indeed have breaking changes. It's experimental, so it doesn't require a major version bump. The author of yarn was aware of the incoming change (he is on this team).

[localjo]

@JakobJingleheimer It seems that the breaking changes are breaking a lot of people's builds though. See the discussion here: yarnpkg/berry#4778

Whether it should have been merged or not, I don't know. But I'm mentioning it here in case people are looking for info about why their projects stopped building, and how to solve it. In my case, I solved my issue by locking Node in my CI environment to 16.16.0. Less than ideal, but it works for me.

[GeoffreyBooth]

Whether it should have been merged or not, I don't know.

It was an intentional breaking change. Loaders are still an experimental API.

[localjo]

Fair enough. It seems a handful of packages must be using that experimental API and the people (including myself) who are importing those packages are unaware. Likely either an RTFM problem on my part, or lack of documentation on the part of the publishers of the packages. Anyway, thanks for the clarification.

[wbt]

Breaking changes, especially intentional ones, should not be merged without a breaking-change semver update. This also broke my build process, with GitHub Actions automatically incorporating the latest version of Node. Code that previously worked with no indications about it being experimental was suddenly both labeled as experimental and broken, without a corresponding change in the codebase.

[JakobJingleheimer]

@wbt you are mistaken. This feature has been marked experimental since its inception multiple years ago. This change was also highlighted very prominently at the very top of the release notes as breaking and specifically how and how to update. Experimental features are not semver major.

If your build process is sensitive to experimental changes, you should pin your version appropriately. For example, we do not release breaking experimental changes in semver patch, only semver minor (and if it so happens, major).

If you are dependent on some intermediary that is leveraging an experimental feature and did not pin or release appropriately, take it up with that dependency.

EDIT: If you would like to be notified of notable and breaking changes in Loaders/Customisation API, please subscribe to this issue nodejs/loaders#103