esm: add initialize hook, integrate with register

[izaakschroeder]

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.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/modules

[izaakschroeder]

/cc @@targos @giltayar

[GeoffreyBooth]

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.

[izaakschroeder]

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

[izaakschroeder]

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?

[GeoffreyBooth]

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

[izaakschroeder]

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

[GeoffreyBooth]

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.

[izaakschroeder]

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

[GeoffreyBooth]

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.

[izaakschroeder]

Oh, sorry, removing skip 😅

[JakobJingleheimer]

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)

[JakobJingleheimer]

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]

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

[targos]

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

[aduh95]

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.

[GeoffreyBooth]

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.

[ruyadorno]

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 😊