-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
module: add registerCondition API for runtime-specific conditional exports #59005
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Review requested:
|
I think this can already be solved by something like this? const { registerHooks } = require('module');
registerHooks({
resolve(specifier, context, nextResolve) {
return nextResolve(specifier, {
...context,
conditions: ['electron', ...context.conditions]
});
}
}); ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think this is too much for just applying a new condition. Do we need this utility for a very low freq behavior? I think maybe no
I don't know anything about the conditions feature. If I were looking for a way to do this, I certainly wouldn't come up with this snippet. Instead of relying on users to be intimately familiar with hooks and resolvers, why not merge this PR for an ergonomic API? |
It's hard to say how much more ergonomic this is, for example if users want to apply the conditions differently based on more information about the module request (format of the module, import attributes, information about the parent module etc.) then the current API doesn't provide a way for them to have more powerful control. I am inclined that we can just add an example in the documentation and see how strongly users still need the current shape of this API (from what I can tell though, the code here doesn't actually work, e.g. there is an undefined variable |
Why not make these suggestions as improvements to the API? My point being, instead of suggesting "just do it this way" and providing a snippet only the enlightened will ever know about, improve the contribution and Node's API. |
I think the more it gets improved, the more it will just look like... the resolve hook itself. Hence the suggestion that we can probably just turn it into a documentation change, and see how strongly people still need a subset of the functionality that |
I like this PR, small, self-contained and easy to use. @joyeecheung I see your reasoning but I also tend to agree to with @jsumners-nr. People not really into modules will never come up with that snippet and I also don't see how they will ever find it in the docs unless we add a specific paragraph for this use case. About your point that this API is fairly limited I agree but I also think it's fine like this. @Momena-akhtar Can you please add docs and tests? |
Surely! |
That is precisely what I suggested: add a paragraph to the doc that's basically implementing this API using the hooks. const { registerHooks } = require('module');
function registerCondition(fn) {
registerHooks({
resolve(specifier, context, nextResolve) {
return nextResolve(specifier, {
...context,
conditions: fn(specifier, context)
});
}
});
}
// Usage
registerCondition((specifier, { conditions }) =>
process.versions.electron ? conditions.concat(['electron']) : conditions)); If people have to discover this API through the docs anyway, we can just put this helper in the docs directly for them to discover. This is not too different than all the other examples in the docs that we have, and would be actually fewer lines of docs to write or read compared to formalizing this wrapper as an API. If there's enough request to put these lines directly into the API, we can revisit adding it to the API. But so far it seems more like an underdocumentation issue from the original requests. FWIW even if we need to add this API to core right now I think using above lines would be a lot easier to maintain than the current approach in this PR, using another set scattered elsewhere would make it harder to consolidate the conditions internally and adds one more file to deserialise which impacts startup performance. Also with the CJS path in this PR the number of invocations of the callback per resolution is in proportion to the number of lookup paths (e.g. if 5 node_module folders are searched for a a particular resolution, the callback gets invoked 5 times, then the docs need to sufficiently document this caveat to prevent unwanted side effects in the callbacks), using the hook it's once per resolution. |
I agree with Joyee, this could be a doc PR, and/or an npm package. Adding such specific API to core does increase the maintenance burden, for a relatively niche usecase. |
I have seen a few issues in the past week related to improving support for these conditions. It doesn't seem to be that "niche." Instead, it seems like the API around them is lacking. |
What does this enable that can't be done with the |
@Momena-akhtar As I don't have strong opinion on this and I trust @joyeecheung judgement, would you be satisfied by switching this to be a documentation PR? |
Yes sure, no problem with that! :) |
I've replaced this feature PR with a cleaner documentation-only PR at #59066 - closing this in favor of that. |
Summary
This PR adds support for dynamic runtime condition registration in Node.js module resolution.
It introduces a public API:
Library or runtime authors can use this to register custom resolution conditions (e.g. "electron", "bun", etc.), without needing to modify Node.js core.
Example Usage
Then run:
Now
electron
will be considered during resolution when evaluatingexports
maps.Why
Hardcoding specific runtime conditions (like
electron
) in core does not scale. This approach enables runtime-specific support to be added entirely in userland or preload scripts.This has been requested in:
electron
conditional exports electron/electron#47670This PR provides a generic, extensible solution to support such runtime conditions cleanly without requiring changes to Node.js core for each use case.
Changes
lib/internal/modules/conditions.js
to manage a runtime condition registryresolve.js
) and CommonJS (loader.js
) module resolutionregisterCondition
via the publicnode:module
APIChecklist
make -j4 test
passesregisterCondition
) added tonode:module
Label Suggestion
If accepted, this PR should be labeled:
notable-change
This is a notable feature as it introduces a new public API (
registerCondition
) that enables library authors and runtime environments to define their own conditions for conditional exports resolution in a clean, dynamic way.