n-api: remove n-api module loading flag

Remove the command line flag that was needed for N-API module loading.
Re: nodejs/vm#9

PR-URL: nodejs#14902
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <digitalinfinity@gmail.com>

doc/api/cli.md

@@ -119,14 +119,6 @@ added: v6.0.0
 
 Silence all process warnings (including deprecations).
 
-### `--napi-modules`
-<!-- YAML
-added: REPLACEME
--->
-
-Enable loading native modules compiled with the ABI-stable Node.js API (N-API)
-(experimental).
-
 ### `--trace-warnings`
 <!-- YAML
 added: v6.0.0
@@ -361,7 +353,6 @@ Node options that are allowed are:
 - `--debug-brk`
 - `--debug-port`
 - `--debug`
-- `--napi-modules`
 - `--no-deprecation`
 - `--no-warnings`
 - `--openssl-config`

doc/api/n-api.md

@@ -63,14 +63,6 @@ For example:
 #include <node_api.h>
 ```
 
-As the feature is experimental it must be enabled with the
-following command line
-[option](https://nodejs.org/dist/latest-v8.x/docs/api/cli.html#cli_napi_modules):
-
-```bash
---napi-modules
-```
-
 ## Basic N-API Data Types
 
 N-API exposes the following fundamental datatypes as abstractions that are

src/env-inl.h

@@ -223,6 +223,7 @@ inline Environment::Environment(v8::Local<v8::Context> context,
       using_domains_(false),
       printed_error_(false),
       trace_sync_io_(false),
+      emit_napi_warning_(true),
       makecallback_cntr_(0),
       async_wrap_uid_(0),
       debugger_agent_(this),

src/env.cc

@@ -61,4 +61,10 @@ void Environment::PrintSyncTrace() const {
   fflush(stderr);
 }
 
+bool Environment::EmitNapiWarning() {
+  bool current_value = emit_napi_warning_;
+  emit_napi_warning_ = false;
+  return current_value;
+}
+
 }  // namespace node

src/env.h

@@ -535,6 +535,8 @@ class Environment {
 
   static const int kContextEmbedderDataIndex = NODE_CONTEXT_EMBEDDER_DATA_INDEX;
 
+  bool EmitNapiWarning();
+
  private:
   inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
                          const char* errmsg);
@@ -566,6 +568,7 @@ class Environment {
   bool using_domains_;
   bool printed_error_;
   bool trace_sync_io_;
+  bool emit_napi_warning_;
   size_t makecallback_cntr_;
   int64_t async_wrap_uid_;
   std::vector<int64_t> destroy_ids_list_;

src/node.cc

@@ -167,9 +167,6 @@ static node_module* modlist_addon;
 static 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;
 
@@ -2503,22 +2500,17 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
     env->ThrowError("Module did not self-register.");
     return;
   }
-  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 ABI-stable Node.js API (N-API)."
-               "\nThis feature is experimental and must be enabled on the "
-               "\ncommand-line by adding --napi-modules.",
-               *filename);
-    } else {
-      snprintf(errmsg,
-               sizeof(errmsg),
-               "Module version mismatch. Expected %d, got %d.",
-               NODE_MODULE_VERSION, mp->nm_version);
+  if (mp->nm_version == -1) {
+    if (env->EmitNapiWarning()) {
+      ProcessEmitWarning(env, "N-API is an experimental feature and could "
+                         "change at any time.");
     }
+  } else if (mp->nm_version != NODE_MODULE_VERSION) {
+    char errmsg[1024];
+    snprintf(errmsg,
+             sizeof(errmsg),
+             "Module version mismatch. Expected %d, got %d.",
+             NODE_MODULE_VERSION, mp->nm_version);
 
     // NOTE: `mp` is allocated inside of the shared library's memory, calling
     // `uv_dlclose` will deallocate it
@@ -3733,7 +3725,8 @@ static void PrintHelp() {
          "  --throw-deprecation   throw an exception anytime a deprecated "
          "function is used\n"
          "  --no-warnings         silence all process warnings\n"
-         "  --napi-modules        load N-API modules\n"
+         "  --napi-modules        load N-API modules (no-op - option kept for "
+         "                        compatibility)\n"
          "  --trace-warnings      show stack traces on process warnings\n"
          "  --redirect-warnings=path\n"
          "                        write warnings to path instead of stderr\n"
@@ -3980,7 +3973,7 @@ static void ParseArgs(int* argc,
     } else if (strcmp(arg, "--no-deprecation") == 0) {
       no_deprecation = true;
     } else if (strcmp(arg, "--napi-modules") == 0) {
-      load_napi_modules = true;
+      // no-op
     } else if (strcmp(arg, "--no-warnings") == 0) {
       no_process_warnings = true;
     } else if (strcmp(arg, "--trace-warnings") == 0) {
@@ -4879,11 +4872,6 @@ static void StartNodeInstance(void* arg) {
     if (instance_data->use_debug_agent())
       EnableDebug(env);
 
-    if (load_napi_modules) {
-      ProcessEmitWarning(env, "N-API is an experimental feature "
-          "and could change at any time.");
-    }
-
     {
       SealHandleScope seal(isolate);
       bool more;

src/node_api.cc

@@ -846,28 +846,10 @@ void napi_module_register_cb(v8::Local<v8::Object> exports,
   }
 }
 
-#ifndef EXTERNAL_NAPI
-namespace node {
-  // Indicates whether NAPI was enabled/disabled via the node command-line.
-  extern bool load_napi_modules;
-}
-#endif  // EXTERNAL_NAPI
-
 // Registers a NAPI module.
 void napi_module_register(napi_module* mod) {
-  // NAPI modules always work with the current node version.
-  int module_version = NODE_MODULE_VERSION;
-
-#ifndef EXTERNAL_NAPI
-  if (!node::load_napi_modules) {
-    // NAPI is disabled, so set the module version to -1 to cause the module
-    // to be unloaded.
-    module_version = -1;
-  }
-#endif  // EXTERNAL_NAPI
-
   node::node_module* nm = new node::node_module {
-    module_version,
+    -1,
     mod->nm_flags,
     nullptr,
     mod->nm_filename,

test/addons-napi/test_async/test.js

@@ -15,7 +15,7 @@ if (process.argv[2] === 'child') {
   return;
 }
 const p = child_process.spawnSync(
-  process.execPath, [ '--napi-modules', __filename, 'child' ]);
+  process.execPath, [ __filename, 'child' ]);
 assert.ifError(p.error);
 assert.ok(p.stderr.toString().includes(testException));
 

test/addons-napi/test_fatal/test.js

@@ -12,7 +12,7 @@ if (process.argv[2] === 'child') {
 }
 
 const p = child_process.spawnSync(
-    process.execPath, [ '--napi-modules', __filename, 'child' ]);
+  process.execPath, [ __filename, 'child' ]);
 assert.ifError(p.error);
 assert.ok(p.stderr.toString().includes(
-    'FATAL ERROR: test_fatal::Test fatal message'));
+  'FATAL ERROR: test_fatal::Test fatal message'));

test/addons-napi/test_function/test_function.c

@@ -26,7 +26,9 @@ napi_value TestCallFunction(napi_env env, napi_callback_info info) {
   return result;
 }
 
-void TestFunctionName(napi_env env, napi_callback_info info) {}
+napi_value TestFunctionName(napi_env env, napi_callback_info info) {
+  return NULL;
+}
 
 napi_value Init(napi_env env, napi_value exports) {
   napi_value fn1;

test/addons-napi/test_warning/binding.gyp

@@ -0,0 +1,12 @@
+{
+  "targets": [
+    {
+      "target_name": "test_warning",
+      "sources": [ "test_warning.c" ]
+    },
+    {
+      "target_name": "test_warning2",
+      "sources": [ "test_warning2.c" ]
+    }
+  ]
+}

test/addons-napi/test_warning/test.js

@@ -0,0 +1,18 @@
+'use strict';
+
+if (process.argv[2] === 'child') {
+  const common = require('../../common');
+  console.log(require(`./build/${common.buildType}/test_warning`));
+  console.log(require(`./build/${common.buildType}/test_warning2`));
+} else {
+  const run = require('child_process').spawnSync;
+  const assert = require('assert');
+  const warning = 'Warning: N-API is an experimental feature and could ' +
+                  'change at any time.';
+
+  const result = run(process.execPath, [__filename, 'child']);
+  assert.deepStrictEqual(result.stdout.toString().match(/\S+/g), ['42', '1337'],
+                         'Modules loaded correctly');
+  assert.deepStrictEqual(result.stderr.toString().split(warning).length, 2,
+                         'Warning was displayed only once');
+}

test/addons-napi/test_warning/test_warning.c

@@ -0,0 +1,11 @@
+#include <node_api.h>
+#include "../common.h"
+
+napi_value Init(napi_env env, napi_value exports) {
+  napi_value result;
+  NAPI_CALL(env,
+    napi_create_uint32(env, 42, &result));
+  return result;
+}
+
+NAPI_MODULE(test_warning, Init)

test/addons-napi/test_warning/test_warning2.c

@@ -0,0 +1,11 @@
+#include <node_api.h>
+#include "../common.h"
+
+napi_value Init(napi_env env, napi_value exports) {
+  napi_value result;
+  NAPI_CALL(env,
+    napi_create_uint32(env, 1337, &result));
+  return result;
+}
+
+NAPI_MODULE(test_warning2, Init)

test/addons-napi/testcfg.py

@@ -3,4 +3,4 @@
 import testpy
 
 def GetConfiguration(context, root):
-  return testpy.AddonTestConfiguration(context, root, 'addons-napi', ['--napi-modules'])
+  return testpy.AddonTestConfiguration(context, root, 'addons-napi')