Repository navigation
ESM Loader Hooks (20.8) should support a more testable, less stateful design #50042
Description
What is the problem this feature will solve?
The current ESM Loader Hooks are defined as modules having the following named exports: initialize(), resolve() and load(). The initialize() function is passed data specified when calling register(). The intent is that the other two hooks can thus also leverage this data in extensible, future-proof ways.
The problem that this introduces is that the ESM Load Hook module is now a stateful singleton because it encourages designs wherein initialization state is shared at the module level. This means that any tests looking to exercise the loader module needs to deal with ESM cache invalidation. It also makes it slightly less ergonomic (and type-safe!) to set and access any needed shared state.
What is the feature you are proposing to solve the problem?
I propose a slight change (or addition) to the proposed design. The change I'd like to propose is that the initialize() function should be able to produce the object implementing the other two components of the interface: resolve() and load().
In this way:
- Tests looking to test the loader's implementation can easily produce multiple instances of the loader for testing different inputs and scenarios in isolation.
- State can be passed in a simple, immutable and type-safe manner between the
initialize()call and theresolve()andload()hooks. - A Loader could then be implemented as a class that is instantiated and returned by the
initialize()function. The class can hold shared state in a private field or instance properties. Alternatively, theinitialize()closure could also be used to hold this state in a way that the two other hooks can easily access.
export async function initialize(data) {
return new MyLoader(data);
}
class MyLoader {
#port;
constructor(data) {
this.#port = data.port;
}
resolve(specifier, context, nextResolve) {
this.#port.postMessage(['resolve', specifier, context]);
// No-op, defer to built-in
return nextResolve(specifier, context);
}
load(url, context, nextLoad) {
this.#port.postMessage(['load', url, context]);
// No-op, defer to built-in
return nextLoad(url, context);
}
}What alternatives have you considered?
No response
Activity
GeoffreyBooth commentedon Oct 4, 2023
@nodejs/loaders
ggoodman commentedon Oct 4, 2023
BTW, I threw #50044 together to explore what a back-compatible approach would look like for this.
ggoodman commentedon Oct 19, 2023
@GeoffreyBooth, here are some higher-level observations about the current loader API:
loadhook doesn't have access to a hookedresolvefunction. This means aloadhook can't, for example, attempt to resolve atsconfig.jsonor other config file using the loader's own machinery. It must use a parallel approach.resolvehook doesn't have access to a hookedloadfunction. Similar to the above, theresolvehook might want to load the content of a config file that would influence module resolution. Using the loader's own machinery would be advantageous over having a parallel construction.initialize,loadandresolvehooks are top-level functions whose only option for extensibility is through new arguments. By extensibility, I mean unforeseen use-cases or data that needs to be passed into the hook. This is not strictly bad but feels like a missed opportunity.a. Sharing state between the
initializeandresolve/loadhooks must rely on module-level state that is hard to evict. This makes testing and de-registering loaders unnecessarily complicated.b. It is difficult to cleanly add type-safety to a loader since the interface is the module. We don't have facilities for asserting that an entire modules's exports adhere to a desired interface.
shortCircuitvsnextResolve/nextLoadinteraction is very confusing and clunky. Why do we needshortCircuit?I've started speccing out what I think an alternative design might look like and as I'm attempting to port the CoffeScript loader from the docs over, I'm seeing some of the motivation for the
nextResolveandnextLoadfunctions. I have run out of time and brain-power trying to make that work today. Hope to pick it up tomorrow.GeoffreyBooth commentedon Oct 20, 2023
@ggoodman I’m warming to the idea of
initializereturning something whoseresolveandloadproperties define those hooks. That can be an elegant way to provide a loader “interface” as you’re describing, and if we still permit the top-level exports then it’s not breaking. This API has been around for a few years now and we’ve gotten some strong pushback the previous times we’ve changed it, so I think at this point all changes should be additive unless there’s a strong motivation otherwise. “We could’ve thought of something better” isn’t quite strong enough in my opinion. But we can support both styles.As for
loadhaving access toresolveand vice versa, they do have access in the sense that either hook’s body can callimport()which would kick off a new module resolution and loading pipeline. They can’t access the resolve portion or the load portion of that pipeline individually, but I’m not sure how it would make sense to do so; how would that work with chained hooks?And for your #5, chaining is the reason we need those things. We want the ability to register multiple sets of hooks, like for example just as people today often do
--require tsconfig-paths/register --require ts-node/registerwe want to support the equivalent via--import/register().isaacs commentedon Oct 20, 2023
@ggoodman
You might find node-tap's loader interfaces informative here.
I've been putting these hook methods in a
hooks.mtsfile (so that I can export one set with initialize/resolve/load, and another with globalPreload/resolve/load), and testing with tap'st.mockImport(). This makes it pretty much the same difficulty to test as it would be to test a regular function, since each invocation has a new unique context and even its own mocked dependencies.The exported types in
@types/nodecover the loader hooks interface pretty comprehensively, and if you have type checking on in your tests, I'm not sure how much more type-tested it could really be. Maybe in a perfect world, node would have a way to verify the types at run time, but that's not how ts works. 🤷♂️giltayar commentedon Oct 20, 2023
If it was just to help with testing, I would not have been for this change, as there are other ways to deal with tests here.
But!
We still have an open issue regarding communication with hook (e.g. an api that changes thing in the hook), when there are multiple worker threads.
If we call
initializefor each worker thread, and each worker thread has their ownportand resolve and initialize, then this is a problem solved as each worker thread will have its own port, and the hook will have their own state per worker thread.So not am I only not for this idea, I think it's a must have to solve an open issue in loaders.
ggoodman commentedon Oct 20, 2023
@isaacs That sounds quite awesome TBH because it's much higher fidelity than the sorts of tests I'm proposing. At the same time, it relies on using a smaller subset of test frameworks that support this sort of isolation. I don't think Node's own test runner even has this sort of advanced functionality yet.
ggoodman commentedon Oct 20, 2023
Agreed @giltayar, I don't think the current design has a good story for instantiation. In other words, the forced reliance on module-level state means that the same loader can't be instantiated multiple times with different configs.
Of course, there are probably some tricky ways to get around this but the current API seems unnecessarily difficult for such use-cases.
ggoodman commentedon Oct 20, 2023
Happy to rebase #50044 and spike out the docs changes if there seems to be momentum in favour of the change.
GeoffreyBooth commentedon Oct 20, 2023
Can the PR add some tests for this? This would be good to have coverage around.
We might want to add an example in the docs for worker threads if supporting them is something that hook authors will need to explicitly enable.
As for the
initializereturn value, I would just like to be able to continue adding to it in the future. So for now maybe it supports only{ resolve: Function; load: Function }but if we discover new needs for it in the future, we can add more properties.Yes please!
github-actions commentedon Apr 18, 2024
There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-staleIssues and PRs exempt from automated stale handling.
label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.
github-actions commentedon May 18, 2024
There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.
For more information on how the project manages feature requests, please consult the feature request management document.