Replace globalPreload

@giltayar

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 😅

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.

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

that'd be amazing! PR?

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.

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.

@targos

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

@targos

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

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

@GeoffreyBooth

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

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.

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?

I’m not sure how I can put the PR on node’s repo

So your first PR is based on your support-nested-loader-chains branch. I suggest that you:

Branch off of that branch in your repo, something like support-nested-loader-chains-and-register-args, so that the two branches both point to the same commit.
Rebase the support-nested-loader-chains branch to drop the newest, register-related commit.
Force-push support-nested-loader-chains to your repo. This will reset the PR to exclude the newest commit.
Open a new PR with support-nested-loader-chains-and-register-args against node:main and mention in the PR description that it builds off esm: unflag Module.register and allow nested loader import() node#48559.

That’s it. This is the general procedure for splitting a large PR into smaller ones.

@izaakschroeder

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:

Why do we have both data and transferList as separate arguments? What’s the difference between them?

@izaakschroeder did you see/use @targos’ POC in #147 (comment)?

esm: add `initialize` hook

esm: add `initialize` hook, integrate with `register`

The question (or is it a thinly-veiled ask? 😁) is this: how can I know whether I already registered the loader or not?

As far as what node will tell you, there's no straightforward way to determine that (ex you can't do registered.has(myLoader)). I can think of a couple heinous ways to figure it out. Where do you need to know–in import's module, in your loader, or from the app/entry-point?

True! But what happens when testdouble.js is loaded twice because it is used in two workers?

Do you mean node's ESM worker? I think there may currently be unintended/undesired behaviour making that possible, but we'll squash that.

if I have two workers that want to communicate with the loader.

What are these workers / where did they come from? Are they spawned by the entry point?

@giltayar

@giltayar maybe? I can help… I built much of the new register machinery so perhaps I could try to build out an example that fits here?

`testdouble-super-register.js`

import {MessageChannel, Worker, isMainThread, workerData} from 'node:worker_threads';
import {register} from 'node:module';

const TestDoubleLock = Symbol();

// > True! But what happens when testdouble.js is loaded twice because it is used in two workers?
// This will prevent testdouble.js from executing `register` multiple times from other workers
// Node _should_ generally only execute a single module once but I people could get likely
// around that if they tried. You could add an additional guard on `globalThis` which is shared
// between every module on a single V8 context (~thread). 
if (isMainThread && !globalThis[TestDoubleLock]) {
  globalThis[TestDoubleLock] = true;

  const controlChannel = new MessageChannel();

  const result = register('testdouble-super-loader.js', {
    parentURL: import.meta.url,
    data: {port: controlChannel.port2},
    transferList: [controlChannel.port2], 
  });

  if (result.error) {
    throw new Error('Somehow registered testdouble twice!');
  }

  const spawnWorker = () => {
    const workerChannel = new MessageChannel(); 
    // > if I have _two_ workers that want to communicate with the loader.
    // You can pass ports around – one that goes to the loader and one to the worker.
    new Worker(import.meta.url, {
       workerData: {port: workerChannel.port1},
       transferList: [workerChannel.port1],
    });
    controlChannel.port1.postMessage(
      {type: 'NEW_WORKER', port: workerChannel.port2}, 
      [workerChannel.port2]
    );
  };
  
  // Do what you need to do.
  spawnWorker();
  spawnWorker();
  spawnWorker();
} else {
  workerData.port.postMessage({
    type: 'THIS_GOES_TO_LOADER'
  });
}

`testdouble-super-loader.js`

const handleMessage = (msg) => {
  switch(msg.type) {
  case 'NEW_WORKER':
    msg.port.on('message', handleMessage);
    break;
  case 'THIS_GOES_TO_LOADER':
    // YOUR LOGIC HERE FROM YOUR WORKERS
    break;
  }
}

// > The question (or is it a thinly-veiled ask? 😁) is this: how can I 
// > know whether I already registered the loader or not?
// You _CAN_ know this here; `initialize` is called each time the 
// loader is registered.
//
// *IMPORTANT*: There is currently no way for this value to be
// mucked up, but if/when `deregister` lands to remove a loader
// then we may need a `deinitialize` hook or similar to keep the
// value here correct.

let lock = 0;

const initialize = (data) => {
  if (lock > 0) {
    // Loader has already been registered!
    return {error: true};
  }
  lock++;
  data.port.on('message', handleMessage);
  return {error: false};
}

For what it's worth I'm also not against exposing a getLoaders() type method on node:module. I don't think it would be hard to implement and we could have it store the results of initialize across the thread bridge.

register(originalSpecifier, parentURL, data, transferList) {
  const id = ''; // TBD: Determine how to pull this out.
  this.loaders[id] = hooksProxy.makeSyncRequest('register', transferList, originalSpecifier, parentURL, data);
  return this.loaders[id];
}

deregister(id) {
  const result = hooksProxy.makeSyncRequest('deregister', undefined, id);
  delete this.loaders[id];
  return result;
}

getLoaders() {
  return Object.values(this.loaders);
}

Would we want to expose the whole registry or would just name and position do the job? Would someone want to use the loader or just know about it?

Would we want to expose the whole registry or would just name and position do the job? Would someone want to use the loader or just know about it?

I'm… not sure… I guess we would probably want to define an interface for what it returns… I assume at least the id… but maybe more.

has() would be safer than getLoaders(), since programs might be relying on the function they pass in not being reachable by userland code (same reason browsers don't have a "get all event listeners" function)

I don't want to create APIs without a clear use case. If loader authors can solve this problem themselves with a trivial amount of code, I'd leave it at that until it becomes clear that the existing solution is insufficient somehow.

esm: add `initialize` hook, integrate with `register`

I think this is super obsolete, so closing.

Navigation Menu

Repository navigation

Replace globalPreload #147

21 remaining items

`testdouble-super-register.js`

`testdouble-super-loader.js`

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Replace globalPreload #147

Description

Activity

giltayar commented on Jun 21, 2023

targos commented on Jun 21, 2023

mcollina commented on Jun 21, 2023

JakobJingleheimer commented on Jun 21, 2023

GeoffreyBooth commented on Jun 21, 2023

mcollina commented on Jul 10, 2023

JakobJingleheimer commented on Jul 10, 2023

GeoffreyBooth commented on Jul 10, 2023

izaakschroeder commented on Jul 10, 2023

GeoffreyBooth commented on Jul 11, 2023

izaakschroeder commented on Jul 11, 2023

GeoffreyBooth commented on Jul 11, 2023

GeoffreyBooth commented on Jul 11, 2023

21 remaining items

JakobJingleheimer commented on Aug 21, 2023

izaakschroeder commented on Aug 21, 2023

testdouble-super-register.js

testdouble-super-loader.js

izaakschroeder commented on Aug 21, 2023

JakobJingleheimer commented on Aug 21, 2023

izaakschroeder commented on Aug 21, 2023

ljharb commented on Aug 21, 2023

GeoffreyBooth commented on Aug 21, 2023

JakobJingleheimer commented on Nov 8, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`testdouble-super-register.js`

`testdouble-super-loader.js`