doc: update module customization hooks docs - #49265
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
|
Maybe we could address #49282 as part of this PR? |
Sorry, something went wrong.
How, exactly? The examples already show the functions’ signatures as being async. Edit: The resolve example was sync, but this PR adds the async keyword even though it’s unnecessary for this particular example (since the example resolve here doesn’t await anything). This PR also adds a an overview of the hooks’ signatures in https://github.com/nodejs/node/pull/49265/files#diff-4e8f3cce79719e4a337f58575b20c998b093eb64164b847ca0eb9ba884d8a801R341-R351, showing them all as async. |
Sorry, something went wrong.
There was a problem hiding this comment.
Should we keep calling it the loader thread?
Sorry, something went wrong.
There was a problem hiding this comment.
I don't think so. It's the thread where the hooks run. One term is better than two.
Sorry, something went wrong.
There was a problem hiding this comment.
Orchestrator/orchestration threads?
Sorry, something went wrong.
There was a problem hiding this comment.
I really don't like "hooks threads", especially if the goal is to make room for hooks in other core modules. Let's keep "loader thread" until we find a better name?
| ### Communication between main and hooks threads | |
| ### Communication between main and loader threads |
Sorry, something went wrong.
There was a problem hiding this comment.
Customization thread?
I wasn't assuming we'd need a thread like this (or use this one) for other systems, but who knows?
Sorry, something went wrong.
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
There was a problem hiding this comment.
I really don't like "hooks threads", especially if the goal is to make room for hooks in other core modules. Let's keep "loader thread" until we find a better name?
| ### Communication between main and hooks threads | |
| ### Communication between main and loader threads |
Sorry, something went wrong.
| `resolve`); if `resolve` provides a `format`, a custom `load` hook is required | ||
| even if only to pass the value to the Node.js default `load` hook. |
There was a problem hiding this comment.
While we're editing this, we should consider rephrase that, I don't understand what it means.
Sorry, something went wrong.
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #49265 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#49265 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#49265 Backport-PR-URL: nodejs/node#50669 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs/node#49265 Backport-PR-URL: nodejs/node#50669 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
This PR builds on and follows up #49261.
The aim here was to implement most of this to-do list. In particular:
I hopefully removed all references to the “Loaders API” and similar nomenclature, in favor of “module customization hooks” or “hooks” or similar.
I moved most of the “how to use this API” text out of the module.register() section and into a new section “Enabling” under the “Customization Hooks” heading.
I removed all references to --experimental-loader except in cli.md, rewriting to recommend --import with register() instead. We’re considering removing the --experimental-loader flag, but for now at least we want to discourage its use. (We anticipate that most third-party libraries like ts-node will need code on the main thread, which --import with register() can enable but --experimental-loader cannot, so people should get familiar with the more powerful approach and we don’t need a secondary, less capable method.)
I added CommonJS versions of all code samples that could support them (so anything running on the main thread, since the hooks module needs to export specific named functions).
I classified the overall Module Customization Hooks API as stability level “1.1 - Active development” and the resolve and load hooks as “1.2 - Release candidate,” and globalPreload as “1.0 - Early development” (since I can’t mark it as deprecated, but its end is near whenever esm: remove globalPreload hook (superseded by initialize) #49144 lands).
I added some mentions that parts of this work for CommonJS modules and require as well.
The CoffeeScript example seemed to have gotten out of date, so I rewrote it to how I think it should be now that CommonJS is supported.
Miscellaneous cleanup.
What’s left for follow-up PRs:
Remove the globalPreload section as part of esm: remove globalPreload hook (superseded by initialize) #49144.
Remove the mention of --experimental-loader from cli.md?
Update --experimental-loader to print a warning that it may be removed in the future, and users should use --import with register instead; this warning could even generate example code that shows --import with a data URL that includes the value of what was passed to --experimental-loader (similar to how we have a fancy error message with suggested new code for unknown named exports from CommonJS modules).
Also fixes #49282.
cc @nodejs/loaders @nodejs/modules @aduh95 @JakobJingleheimer