Replace globalPreload

[JakobJingleheimer]

This hook is extremely confusing, even for maintainers.

The replacement needs to address:

• cross-thread communication
• determining what the entry-point is (since it's stripped from process.argv) as identified by ESM loader v20.3.0 access parent path help#4190 (eg an ESM equivalent of require.main)

Assuming there is 1 loader worker.

Spit-balling ideas:

• expose a message port on node:module
• return a message port from register()

Will a message port need to be refreshed?

@giltayar,I would be very interested in your thoughts (or even a design proposal) for this, especially because you recently handled this in testdouble, so the pain is fresh 😅

[giltayar]

The most intuitive way for me is that the register returns a port that is used to communicate with this loader (how is this done today? It can't be the same port for all the loaders, and yet they all run in the same worker). In this option, we would have a function in the loader (initialize) which will accept the port to be used to communicate.

Another variant of this option is for the register option to accept a MessageChannel (instead of creating it and returning it), which the user of register can use to communicate with the loader. The same initialize function can be used (we can even call globalPreload with the port for backward compatibility! 😁). The nice thing about this option is that if the user doesn't pass the MessageChannel to the register, then it signals that there is no need to call initialize and no need for MessagePort support.

In addition to passing a MessageChannel, the register should allow passing any data whatsoever to the initialize, e.g. the userland code can now pass process.argv to the loader to satisfy the requirement in nodejs/help#4190. And actually, the user of register can just create a MessageChannel, pass the port2 via this data (it is a transferable object) and that's it: no need for any Node.js code for ports.

To summarize the above and define what is for me the best option: 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.

[targos]

I like that idea! I made a quick PoC here: targos/node@c4f5e91

$ ./node --no-warnings --loader "data:text/javascript," poc.mjs
initialize loader { entryPoint: '/Users/mzasso/git/nodejs/node/poc.mjs' }
loader initialized

[mcollina]

that'd be amazing! PR?

[JakobJingleheimer]

Sweeeet!

Lemme get the existing register PR stable (I think there are 1–2 small issues with it remaining), and then let's get that in 🎉

PS haven't had time to thoroughly review Gil's ideas, but I liked where they're going.

[GeoffreyBooth]

One thing to consider is that the point of the communications port isn’t just to pass data at initialization time. There are plenty of use cases where loaders need to communicate back and forth to the main thread after initialization.

Now maybe all this means is that we should rename initialize to something like onMessage, if it runs on every message, or initialize could return a function that runs on every message. But we should handle this use case in the design somehow.

Edit: I just looked at the POC and I see the intent, that you’re passing in the port and can register callbacks to it independently from us needing to provide infrastructure for them. I think it can work.

Before converting this into a full-fledged PR, can someone update the docs in the POC so that I can see what the intended UX is supposed to be? I think it would be good to workshop that before someone goes and implements it. What I think is the proposed UX seems fine to me at first glance, but having it spelled out in the instructions we would write for loader authors would help me fully understand it.

[mcollina]

@targos @GeoffreyBooth how is this going? I think this is blocking a few use cases to fully support Node.js v20.

Happy to help in shipping this.

[JakobJingleheimer]

@targos @GeoffreyBooth how is this going? I think this is blocking a few use cases to fully support Node.js v20.

Happy to help in shipping this.

I just got back from vacation.

Before I left, there appeared to be 1 issue in nodejs/node#48439 related to sequence that I didn't have time to resolve. Picking back up this week (you're welcome to take a gander at it too).

PS It looks like a duplicate PR attempting to do the same thing was started whilst I was away. I haven't had a chance yet to look at it.

[GeoffreyBooth]

PS It looks like a duplicate PR attempting to do the same thing was started whilst I was away. I haven't had a chance yet to look at it.

nodejs/node#48559

[izaakschroeder]

@GeoffreyBooth @JakobJingleheimer I updated my PR to address this issue 😄 I followed the proposal from @giltayar and the signature for register is updated to be able to pass arbitrary data to a loader's initialize hook, including two additional tests demonstrating the capability. This change exists in its own commit in order to be able to be reviewed separately.

Note, however, that I opted to add an additional specific transferList parameter instead of attempting to automatically deeply inspect the sent data, so the signature for register is now:

register(loader: string, parentUrl?: string, data?: any, transferList?: any[]): unknown;

The return value from the initialize hook is sent back, but there is currently no affordance for this to also provide a transferList and thus ports or other shared buffers cannot currently be returned from initialize.

I did not remove globalPreload as I am not yet comfortable enough knowing which bits and pieces are safe to pull out or if a deprecation notice is preferred over simply deleting the whole thing.

[GeoffreyBooth]

This change exists in its own commit in order to be able to be reviewed separately.

Thank you so much! Do you mind opening this as its own PR? And it can include/depend on the earlier one. And the earlier PR can exclude the new commit so that it can be reviewed on its own.

[izaakschroeder]

Done!

PR is here: izaakschroeder/node#1 I'm not sure how I can put the PR on node's repo without the base branch also living in node's repo 🤔 Would you prefer I just target node:main instead and include the previous commit? Or?