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

Theregister API is now:

interfaceOptions{parentUrl?:string;data?:any;transferList?:any[];}functionregister(loader:string,parentUrl?:string):any;functionregister(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 newinitialize hook. If this hook returns data it is passed back toregister:

functioninitialize(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.

Copy link

Collaborator

nodejs-github-bot commentedJul 19, 2023

Review requested:

@nodejs/loaders
@nodejs/modules

nodejs-github-bot added esm

Issues and PRs related to the ECMAScript Modules implementation.

needs-ciPRs that need a full CI run. labels

Jul 19, 2023

izaakschroeder marked this pull request as ready for review

July 19, 2023 22:03

Copy link

ContributorAuthor

izaakschroeder commentedJul 19, 2023

/cc @@targos @giltayar

anonrig reviewed

Jul 19, 2023

View reviewed changes

lib/internal/modules/esm/loader.jsShow resolvedHide resolved

lib/internal/modules/esm/loader.js OutdatedShow resolvedHide resolved

lib/internal/modules/esm/hooks.js OutdatedShow resolvedHide resolved

ljharb reviewed

Jul 19, 2023

View reviewed changes

lib/internal/modules/esm/hooks.js OutdatedShow resolvedHide resolved

Copy link

Member

GeoffreyBooth commentedJul 19, 2023

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 needparentURL? It seems like it’s already optional and set toprocess.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. Sinceregister should work in CommonJS too, it would be good to have both CommonJS and ESM versions of the examples.

GeoffreyBooth added the loadersIssues 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 20TypeStrong/ts-node#2009

Open

aduh95 reviewed

Jul 20, 2023

View reviewed changes

lib/internal/modules/esm/hooks.js OutdatedShow resolvedHide resolved

aduh95 reviewed

Jul 20, 2023

View reviewed changes

test/fixtures/es-module-loaders/hooks-initialize.mjs OutdatedShow resolvedHide resolved

aduh95 reviewed

Jul 20, 2023

View reviewed changes

lib/internal/modules/esm/hooks.js OutdatedShow resolvedHide resolved

aduh95 reviewed

Jul 20, 2023

View reviewed changes

lib/internal/modules/esm/hooks.js OutdatedShow resolvedHide resolved

aduh95 reviewed

Jul 20, 2023

View reviewed changes

lib/internal/modules/esm/loader.js OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

izaakschroeder commentedJul 21, 2023

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

Copy link

ContributorAuthor

izaakschroeder commentedJul 21, 2023

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?

Copy link

Member

GeoffreyBooth commentedJul 21, 2023

how/where do I update the docs?

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

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

Copy link

ContributorAuthor

izaakschroeder commentedJul 22, 2023

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

Copy link

Member

GeoffreyBooth commentedJul 22, 2023

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 likenode --require ts-node/register app.js and that should work as expected. Bonus points for setting up arequire.extensions hook that does something and it's coordinated with theload hook. That simulates the transpilation use case.

Copy link

ContributorAuthor

izaakschroeder commentedJul 22, 2023

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

Copy link

Member

GeoffreyBooth commentedJul 22, 2023

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

Copy link

ContributorAuthor

izaakschroeder commentedJul 22, 2023

Oh, sorry, removing skip 😅

JakobJingleheimer reviewed

Jul 22, 2023

View reviewed changes

Copy link

Member

JakobJingleheimer left a comment

Choose a reason for hiding this comment

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

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)

lib/internal/modules/esm/hooks.js OutdatedShow resolvedHide resolved

lib/internal/modules/esm/hooks.jsShow resolvedHide resolved

lib/internal/modules/esm/hooks.js OutdatedShow resolvedHide resolved

lib/internal/modules/esm/hooks.jsShow resolvedHide resolved

lib/internal/modules/esm/loader.js OutdatedShow resolvedHide resolved

test/es-module/test-esm-loader-hooks.mjs OutdatedShow resolvedHide resolved

Copy link

Member

JakobJingleheimer commentedJul 22, 2023

Do we needparentURL?

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

JakobJingleheimer mentioned this pull request

Jul 22, 2023

ESM Loaders breaking changes notificationnodejs/loaders#103

Open

Copy link

Member

GeoffreyBooth commentedJul 22, 2023

@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 forspecifier relative to the importing file. Assuming that’s the reason, I still think I’d like to removeparentUrl from the signature. It defaults toprocess.cwd, so I think we could eliminate it and anyone whowould have been passing it (becausespecifier is relative to some base other than CWD) can instead callregister(new URL(specifier, parentURL).href, data, transferList).

And ifspecifier cannot be found, the error message should say something about Node searching for itrelative 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 notparentUrl stays or goes, as I think this “relative to CWD” behavior is atypical for Node and potentially surprising.

How do others feel about this?

Copy link

Member

targos commentedJul 22, 2023

I would prefer the signature to beloader, options?.

Copy link

Contributor

aduh95 commentedJul 22, 2023•
edited
Loading

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

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

becausespecifier 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 ofregister calls.

Copy link

Member

GeoffreyBooth commentedJul 22, 2023

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

For a package you would just pass the specifier, e.g.register('ts-node'). My example was only for custom loaders, likeregister(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 howregister 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 howimport() works, if anyone can figure out how to make that happen.