module: add support for abi stable module API #11975
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -180,6 +180,9 @@ static const char* trace_enabled_categories = nullptr; | |
| std::string icu_data_dir; // NOLINT(runtime/string) | ||
| #endif | ||
|
|
||
| // N-API is in experimental state, disabled by default. | ||
| bool load_napi_modules = false; | ||
|
|
||
| // used by C++ modules as well | ||
| bool no_deprecation = false; | ||
|
|
||
|
|
@@ -2463,15 +2466,24 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) { | |
| } | ||
| if (mp->nm_version != NODE_MODULE_VERSION) { | ||
| char errmsg[1024]; | ||
| if (mp->nm_version == -1) { | ||
| snprintf(errmsg, | ||
| sizeof(errmsg), | ||
| "The module '%s'" | ||
| "\nwas compiled against the Node.js API. This feature is " | ||
| "\nexperimental and must be enabled on the command-line.", | ||
| *filename); | ||
| } else { | ||
| snprintf(errmsg, | ||
| sizeof(errmsg), | ||
| "The module '%s'" | ||
| "\nwas compiled against a different Node.js version using" | ||
| "\nNODE_MODULE_VERSION %d. This version of Node.js requires" | ||
| "\nNODE_MODULE_VERSION %d. Please try re-compiling or " | ||
| "re-installing\nthe module (for instance, using `npm rebuild` " | ||
| "or `npm install`).", | ||
| *filename, mp->nm_version, NODE_MODULE_VERSION); | ||
| } | ||
|
|
||
| // NOTE: `mp` is allocated inside of the shared library's memory, calling | ||
| // `uv_dlclose` will deallocate it | ||
|
|
@@ -3535,6 +3547,7 @@ static void PrintHelp() { | |
| " --trace-deprecation show stack traces on deprecations\n" | ||
| " --throw-deprecation throw an exception on deprecations\n" | ||
| " --no-warnings silence all process warnings\n" | ||
| " --napi-modules[=yes|no] load N-API modules (default no)\n" | ||
|
There was a problem hiding this comment. As the comment at the beginning of this function says: // XXX: If you add an option here, please also add it to doc/node.1 and
// doc/api/cli.mdThere was a problem hiding this comment. I think this would be our only option that has a There was a problem hiding this comment. If somebody really wants to avoid loading N-API modules even after we've eventually set the default to "yes", thus rendering the use of the command line option unnecessary, they will be unable to do so unless we give them the option to say "no". Of course, if the consensus is that we do not need to provide users with a way to explicitly disable the support for N-API modules, then the option can be reducded to There was a problem hiding this comment.
I'm not sure that's something that we'll need to support at that point. If N-API is successful it should become the normal way of writing native modules, so it wouldn't really make sense to disable it. There was a problem hiding this comment. I agree. Once non-experimental we would would most likely just remove the flag. I think at this point we could just likely make it --napi-modules with no options. There was a problem hiding this comment. I personally would prefer There was a problem hiding this comment. @Fishrock123 can you explain why you (or someone) might want to disable the feature after it is out of experimental status? There was a problem hiding this comment. No, actually. I really do not know, but I would prefer to not have |
||
| " --trace-warnings show stack traces on process warnings\n" | ||
| " --redirect-warnings=path\n" | ||
| " write warnings to path instead of\n" | ||
|
|
@@ -3705,6 +3718,12 @@ static void ParseArgs(int* argc, | |
| force_repl = true; | ||
| } else if (strcmp(arg, "--no-deprecation") == 0) { | ||
| no_deprecation = true; | ||
| } else if (strcmp(arg, "--napi-modules") == 0) { | ||
| load_napi_modules = true; | ||
| } else if (strcmp(arg, "--napi-modules=yes") == 0) { | ||
| load_napi_modules = true; | ||
| } else if (strcmp(arg, "--napi-modules=no") == 0) { | ||
| load_napi_modules = false; | ||
| } else if (strcmp(arg, "--no-warnings") == 0) { | ||
| no_process_warnings = true; | ||
| } else if (strcmp(arg, "--trace-warnings") == 0) { | ||
|
|
@@ -4473,6 +4492,11 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, | |
| if (debug_enabled) | ||
| EnableDebug(&env); | ||
|
|
||
| if (load_napi_modules) { | ||
| ProcessEmitWarning(&env, "N-API is an experimental feature " | ||
| "and could change at any time."); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. I’m not a fan of printing warnings like these… if the user opted into the feature, then they opted in because they know what they were asking for. There was a problem hiding this comment. CTC has specifically requested command-line enablement along with a warning message for features with experimental status. Tracing does the same thing. There was a problem hiding this comment. Is there a link somewhere to that decision? I don't quite understand the justification for a printed warning for a feature that can't even be used without a flag. If you're using the flag, you wanted the feature and are quite aware that it is experimental. Do any V8 features do this? There was a problem hiding this comment. VM Summit 3 meeting notes has this There was a problem hiding this comment. If consensus is that documentation of experimental status is sufficient and this warning message is not necessary, we can take it out. I don't recall anyone feeling very strongly about this at the VM summit. There was a problem hiding this comment. @jasongin I’d say take it out, and we can re-visit that in the unlikely case anybody complains. There was a problem hiding this comment. I think the main motivation was to ensure that anybody turning it on is fully aware of the status and to be consistent with what we have done for other experimental features. |
||
| { | ||
| SealHandleScope seal(isolate); | ||
| bool more; | ||
|
|
||
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 error message could be a bit more specific than
the Node.js API, I’d say. Also, maybe display the flag name here directly?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.
Resolved by b129624