doc: clarify options for single customization hooks thread

[dygabo]

triggered by comment on #47747. This PR adds clarification on single customization chain thread that were introduced with #52706

@nodejs/loaders

[dygabo]

The more I read this, the more that I think it’s unnecessary. Now that we’ve fixed it so that registered hooks apply to all threads, I don’t think we need to call out that it does work as expected. Before the recent fix it would’ve been good to document this unexpected behavior, but we’ve fixed it.

do you mean by that the whole PR is not needed? Because I think it sets expectentions for the user correct.ly. One might expect they create a new Worker and pass in execArgv an --import file.mjs that registers a customization hook and that this has effect on the Worker thread. It is nowhere documented otherwise without this patch. Same for --experimental-loader in fact. And even if it is how we currently implemented it and how we expect it to behave, I think we should document it for the users to avoid misunderstandings. wdyt? Is the behaviour of the customization hoooks explicitly documented anywhereelse?

[GeoffreyBooth]

One might expect they create a new Worker and pass in execArgv an --import file.mjs that registers a customization hook and that this has effect on the Worker thread.

There are probably lots of runtime flags that don’t apply to new Worker. I think it would be better to update our docs more generally to say either something nonspecific like “not all runtime flags apply to worker threads” or to clarify the full list of which ones do or don’t. It doesn’t make sense to single out these in particular, as --import is a very weird one to want to use (you’re spawning a particular file, so why would you want to have another file run beforehand?) and --experimental-loader is de facto deprecated and might get removed.

Alternatively, rather than documenting things, we could update the runtime behavior to help users avoid footguns. So if --import and/or --experimental-loader have no effect in new Worker, then we should throw an error when they’re passed. That would be more effective than documentation, and we wouldn’t even need to document this. It’s arguably a bug that Node just silently ignores these flags currently, or we could land the error as a semver-major change.

[dygabo]

then we should throw an error when they’re passed. That would be more effective than documentation...

we tried that as part of #52706 and we decided to take that up in an own follow-up as it is with current implementation of workers and other implementation details not easy either way (error or success of these operations). As long as this is not solved this behaviour imo should be documented. execArgv does have a list of options that will not work on Workers documented. This update states only that --import and --loader fall into the category of options that affect the process and not the thread. I don't want to argue too much about it as it's a minor thing but imo one that could save some debugging time for people running in this issue with different expectations until we have a runtime solution for it.

EDIT link to the relevant part of t he discussion on #52706

[dygabo]

hi @mcollina , fwiw, this was discussed on the original PR and it was decided to implement a solution in a followup. The loaders work on the worker threads. The configuration of the loader hooks (or customization hooks) is done once per process for the main thread but the hooks are active for all user threads that get spawned after the call to module.register. This PR tries to clarify this for the users running in this issue until it gets properly solved.

[mcollina]

I think we should actually revert that change, or at least revert it in v22 until that follow-up PR is done. https://github.com/nodejs/node/pull/53183.

[GeoffreyBooth]

I’m actually not on board with this change. I think the loaders should be working within worker threads themselves.

As in, you think that users should need to define the hooks that should apply to worker threads, rather than all threads inheriting the hooks registered for the overall process? While per-thread hooks may be useful in certain scenarios, consider the default use case of an application developer writing their app in TypeScript, where that app uses worker threads. They should be able to launch their app via node --import tsx app.ts and spawn workers via new Worker("./worker.ts"); they shouldn’t need to write new Worker("./worker.ts", { execArgv: [ "--import", "tsx" ] }). Needing to do the latter means that the user can’t write portable code; it’s explicitly tied to Node command-line options. Even if someday they want to change their transpilation library, they would need to know to update it not just in the command that launches their app but also in all the places in their code where they call new Worker.

[mcollina]

I think that https://github.com/nodejs/node/pull/52706 breaks (at least) Angular and Ava, and the PR skipped a few tests to pass CI. I'm not on board with that implementation. I think "reusing" a worker should be supported, but that PR was not ready to land.