Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
esm: add experimental support for addon modules #55844
base: main
Are you sure you want to change the base?
esm: add experimental support for addon modules #55844
Changes from 1 commit
1992c20
55efd59
cd68821
d70a87c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I am somewhat surprised to find that there is not a test for the
node-addons
exports condition...it seems the the files pointed to bynode-addons
won't get interpreted as addons unless they have the.node
extension, though this issue predates this PR and also happens torequire()
.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.
Taking a step back, I'm wondering if the format name could be renamed as
node-addon
/node-addons
for alignment.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.
Sure, also I am not even sure if anyone is actually using it, it seems only useful to distinguish "if the current run times can load Node.js style addons" as it currently stands, so may not really be that big of a deal anyway....
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.
I prefer the more concise
addon
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.
Actually I think I misread #55844 (comment) - I think for the format name,
addon
ornode-addon
would make more sense.node-addons
would be a bit weird in the hooks since it's plural.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.
This loader format enum is specific for Node.js so I'll stick with the concise
addon
.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.
In any case, the exports condition question is separate - it's more about when a module that doesn't end with
.node
is pointed to bynode-addons
, should it be interpreted(translated) into an addon or should it be interpreted based on whatever extension it has - and if it has a different extension (e.g. dylib or dll), what should happen then. As I said in the first comment, it's fine to keep it as is since CJS also only considers.node
, but it is a bit strange for the exports condition.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.
The reality is that the conditional exported entries are been loaded based on their extensions, not their entry types. With
require(esm)
, it is possible to do the following already 👀:Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.