-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Merged
+331
−26
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,3 +5,4 @@ Makefile | |
*.mk | ||
gyp-mac-tool | ||
/*/build | ||
/esm/node_modules/*/build |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
#include <node.h> | ||
#include <uv.h> | ||
#include <v8.h> | ||
|
||
static void Method(const v8::FunctionCallbackInfo<v8::Value>& args) { | ||
v8::Isolate* isolate = args.GetIsolate(); | ||
args.GetReturnValue().Set( | ||
v8::String::NewFromUtf8(isolate, "hello world").ToLocalChecked()); | ||
} | ||
|
||
static void InitModule(v8::Local<v8::Object> exports, | ||
v8::Local<v8::Value> module, | ||
v8::Local<v8::Context> context) { | ||
NODE_SET_METHOD(exports, "default", Method); | ||
} | ||
|
||
NODE_MODULE_CONTEXT_AWARE(Binding, InitModule) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
#include <node.h> | ||
#include <uv.h> | ||
#include <v8.h> | ||
|
||
static void InitModule(v8::Local<v8::Object> exports, | ||
v8::Local<v8::Value> module_val, | ||
v8::Local<v8::Context> context) { | ||
v8::Isolate* isolate = context->GetIsolate(); | ||
v8::Local<v8::Object> module = module_val.As<v8::Object>(); | ||
module | ||
->Set(context, | ||
v8::String::NewFromUtf8(isolate, "exports").ToLocalChecked(), | ||
v8::String::NewFromUtf8(isolate, "hello world").ToLocalChecked()) | ||
.FromJust(); | ||
} | ||
|
||
NODE_MODULE_CONTEXT_AWARE(Binding, InitModule) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
#include <node.h> | ||
#include <uv.h> | ||
#include <v8.h> | ||
|
||
static void Method(const v8::FunctionCallbackInfo<v8::Value>& args) { | ||
v8::Isolate* isolate = args.GetIsolate(); | ||
args.GetReturnValue().Set( | ||
v8::String::NewFromUtf8(isolate, "world").ToLocalChecked()); | ||
} | ||
|
||
static void InitModule(v8::Local<v8::Object> exports, | ||
v8::Local<v8::Value> module, | ||
v8::Local<v8::Context> context) { | ||
NODE_SET_METHOD(exports, "hello", Method); | ||
} | ||
|
||
NODE_MODULE_CONTEXT_AWARE(Binding, InitModule) |
Oops, something went wrong.
Oops, something went wrong.
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.
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 👀: