esm: add `initialize` hook, integrate with `register` by izaakschroeder · Pull Request #48842 · nodejs/node · GitHub

Follows @giltayar's proposed API:

register can pass any data it wants to the loader, which will be passed to the exported initialize function of the loader. Additionally, if the user of register wants to communicate with the loader, it can just create a MessageChannel and pass the port to the loader as data.

The register API is now:

interface Options {
  parentUrl?: string;
  data?: any;
  transferList?: any[];
}

function register(loader: string, parentUrl?: string): any;
function register(loader: string, options?: Options): any;

This API is backwards compatible with the old one (new arguments are optional and at the end) and allows for passing data into the new initialize hook. If this hook returns data it is passed back to register:

function initialize(data: any): Promise<any>;

NOTE: Currently there is no mechanism for a loader to exchange ownership of something back to the caller.

Refs: nodejs/loaders#147

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

Review requested:

@nodejs/loaders
@nodejs/modules

/cc @@targos @giltayar

This API is backwards compatible with the old one (new arguments are optional and at the end)

register hasn’t been released yet, so we don’t need to preserve any backward compatibility. That said, the proposed API seems fine. (Do we need parentURL? It seems like it’s already optional and set to process.cwd if undefined, so maybe we can do without it and users can pass as the first argument a path to the loader relative to CWD or an absolute URL to the loader.)

The docs need updating as part of this PR. That would help in reviews, as we can see the intended usage from the example in the new docs. Since register should work in CommonJS too, it would be good to have both CommonJS and ESM versions of the examples.

Thanks for all the feedback! Work is a bit busy, but I will get to this later tonight (Friday) 😄

The docs need updating as part of this PR. That would help in reviews, as we can see the intended usage from the example in the new docs. Since register should work in CommonJS too, it would be good to have both CommonJS and ESM versions of the examples.

@GeoffreyBooth how/where do I update the docs?

how/where do I update the docs?

Edit the module.md file in the repo: https://github.com/nodejs/node/blob/main/doc/api/module.md#moduleregister

You’ll find loaders documented in esm.md: https://github.com/nodejs/node/blob/main/doc/api/esm.md#loaders

@GeoffreyBooth while I do up the docs, is the CJS test here sufficient or do you want to see something more?

is the CJS test here sufficient or do you want to see something more?

We should add a test that specifically uses --require, not just --eval, as the former is a different code path that runs at a different point in startup. Basically, I expect some users to run commands like node --require ts-node/register app.js and that should work as expected. Bonus points for setting up a require.extensions hook that does something and it's coordinated with the load hook. That simulates the transpilation use case.

@GeoffreyBooth I added two more tests but one of them is failing. I added {skip: ...} for now but I will have to do further investigation. Let me know if you have any ideas on the source of this failure 😄

Please don't add skip, we don't want to accidentally land a PR with failing tests. It's okay if CI fails while we're still working.

Oh, sorry, removing skip 😅

This is looking good!

If you'd like help with anything, I have some time this weekend (and then I'm away starting Wednesday for ~2 weeks)

Do we need parentURL?

@GeoffreyBooth yes: this addresses a specific need; I can't remember what it is, but I'm sure it was discussed. I can try to find it if you'd like.

@GeoffreyBooth yes: this addresses a specific need; I can’t remember what it is, but I’m sure it was discussed. I can try to find it if you’d like.

Yes, if you can find the reason, I’d appreciate it. I assume it’s because we can’t figure out how to search for specifier relative to the importing file. Assuming that’s the reason, I still think I’d like to remove parentUrl from the signature. It defaults to process.cwd, so I think we could eliminate it and anyone who would have been passing it (because specifier is relative to some base other than CWD) can instead call register(new URL(specifier, parentURL).href, data, transferList).

And if specifier cannot be found, the error message should say something about Node searching for it relative to the current working directory, rather than relative to the importing file. I think we should probably add this extra error text regardless of whether or not parentUrl stays or goes, as I think this “relative to CWD” behavior is atypical for Node and potentially surprising.

How do others feel about this?

I would prefer the signature to be loader, options?.

can instead call register(new URL(specifier, parentURL).href, data, transferList).

You would not get the expected result if you try to register a node_module package.

because specifier is relative to some base other than CWD

Note that's almost certainly the case, the only case I can think of where resolving relative to the CWD makes sense is to parse a relative path passed as CLI argument, I expect this will not be the case for the vast majority of register calls.

You would not get the expected result if you try to register a node_module package.

For a package you would just pass the specifier, e.g. register('ts-node'). My example was only for custom loaders, like register(new URL('./my-custom-loader.js', import.meta.url).href).

the only case I can think of where resolving relative to the CWD makes sense is to parse a relative path passed as CLI argument

Yes, except that’s how register currently works as far as I know. I assume we all agree that it would be nice if it worked relative to the current file, like how import() works, if anyone can figure out how to make that happen.

Follows @giltayar's proposed API: > `register` can pass any data it wants to the loader, which will be passed to the exported `initialize` function of the loader. Additionally, if the user of `register` wants to communicate with the loader, it can just create a `MessageChannel` and pass the port to the loader as data. The `register` API is now: ```ts interface Options { parentUrl?: string; data?: any; transferList?: any[]; } function register(loader: string, parentUrl?: string): any; function register(loader: string, options?: Options): any; ``` This API is backwards compatible with the old one (new arguments are optional and at the end) and allows for passing data into the new `initialize` hook. If this hook returns data it is passed back to `register`: ```ts function initialize(data: any): Promise<any>; ``` **NOTE**: Currently there is no mechanism for a loader to exchange ownership of something back to the caller. Refs: nodejs/loaders#147 PR-URL: #48842 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Notable changes: doc: * add new TSC members (Michael Dawson) #48841 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 PR-URL: #49185

Notable changes: doc: * add new TSC members (Michael Dawson) #48841 * add rluvaton to collaborators (Raz Luvaton) #49215 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 src: * add built-in `.env` file support (Yagiz Nizipli) #48890 PR-URL: #49185

Notable changes: deps: * V8: cherry-pick 93275031284c (Joyee Cheung) #48660 doc: * add new TSC members (Michael Dawson) #48841 * add rluvaton to collaborators (Raz Luvaton) #49215 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 src: * add built-in `.env` file support (Yagiz Nizipli) #48890 PR-URL: #49185

Notable changes: deps: * V8: cherry-pick 93275031284c (Joyee Cheung) #48660 doc: * add new TSC members (Michael Dawson) #48841 * add rluvaton to collaborators (Raz Luvaton) #49215 esm: * unflag import.meta.resolve (Guy Bedford) #49028 * add `initialize` hook, integrate with `register` (Izaak Schroeder) #48842 * unflag `Module.register` and allow nested loader `import()` (Izaak Schroeder) #48559 inspector: * (SEMVER-MINOR) open add `SymbolDispose` (Chemi Atlow) #48765 module: * implement `register` utility (João Lenon) #46826 * make CJS load from ESM loader (Antoine du Hamel) #47999 src: * add built-in `.env` file support (Yagiz Nizipli) #48890 * initialize cppgc (Daryl Haresign and Joyee Cheung) #48660 and #45704 test_runner: * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 PR-URL: #49185

Based on the note from @GeoffreyBooth on #46826 (comment) I'm going ahead and adding a backport-requested label to this PR too. Ideally you can organize it so that it can be a single backport PR containing all the required commits 😊

PR-URL: #49530 Refs: #48842 Refs: #47999 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Jacob Smith <jacob@frende.me>

Follows @giltayar's proposed API: > `register` can pass any data it wants to the loader, which will be passed to the exported `initialize` function of the loader. Additionally, if the user of `register` wants to communicate with the loader, it can just create a `MessageChannel` and pass the port to the loader as data. The `register` API is now: ```ts interface Options { parentUrl?: string; data?: any; transferList?: any[]; } function register(loader: string, parentUrl?: string): any; function register(loader: string, options?: Options): any; ``` This API is backwards compatible with the old one (new arguments are optional and at the end) and allows for passing data into the new `initialize` hook. If this hook returns data it is passed back to `register`: ```ts function initialize(data: any): Promise<any>; ``` **NOTE**: Currently there is no mechanism for a loader to exchange ownership of something back to the caller. Refs: nodejs/loaders#147 PR-URL: nodejs#48842 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

…5961) **What's the problem this PR addresses?** nodejs/node#48842 changed it so that our fs patch is loaded after the internal translators so ESM importing named CJS exports in loaders doesn't work. Fixes #5951 **How did you fix it?** Updated our ESM loader patching to target the `fs` bindings used internally. **Checklist** - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). - [x] I have set the packages that need to be released for my changes to be effective. - [x] I will check that all automated PR checks pass before the PR gets reviewed.

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Jul 19, 2023

izaakschroeder marked this pull request as ready for review July 19, 2023 22:03

anonrig reviewed Jul 19, 2023

View reviewed changes

ljharb reviewed Jul 19, 2023

View reviewed changes

Comment thread lib/internal/modules/esm/hooks.js Outdated

Comment thread lib/internal/modules/esm/hooks.js Outdated

GeoffreyBooth added the loaders Issues and PRs related to ES module loaders label Jul 20, 2023

GeoffreyBooth mentioned this pull request Jul 20, 2023

fix: Add globalPreload to ts-node/esm for node 20 TypeStrong/ts-node#2009

Open