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: #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>

Author: Gabriel Schulhof

doc/api/cli.md

@@ -178,14 +178,6 @@ added: v8.4.0
 
 Enable the experimental `'http2'` module.
 
-### `--napi-modules`
-<!-- YAML
-added: v8.0.0
--->
-
-Enable loading native modules compiled with the ABI-stable Node.js API (N-API)
-(experimental).
-
 ### `--abort-on-uncaught-exception`
 <!-- YAML
 added: v0.10
@@ -453,7 +445,6 @@ Node options that are allowed are:
 - `--inspect-brk`
 - `--inspect-port`
 - `--inspect`
-- `--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

@@ -288,6 +288,7 @@ inline Environment::Environment(IsolateData* isolate_data,
       printed_error_(false),
       trace_sync_io_(false),
       abort_on_uncaught_exception_(false),
+      emit_napi_warning_(true),
       makecallback_cntr_(0),
 #if HAVE_INSPECTOR
       inspector_agent_(this),

src/env.cc

@@ -213,6 +213,12 @@ bool Environment::RemovePromiseHook(promise_hook_func fn, void* arg) {
   return true;
 }
 
+bool Environment::EmitNapiWarning() {
+  bool current_value = emit_napi_warning_;
+  emit_napi_warning_ = false;
+  return current_value;
+}
+
 void Environment::EnvPromiseHook(v8::PromiseHookType type,
                                  v8::Local<v8::Promise> promise,
                                  v8::Local<v8::Value> parent) {

src/env.h

@@ -669,6 +669,7 @@ class Environment {
 
   void AddPromiseHook(promise_hook_func fn, void* arg);
   bool RemovePromiseHook(promise_hook_func fn, void* arg);
+  bool EmitNapiWarning();
 
  private:
   inline void ThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
@@ -690,6 +691,7 @@ class Environment {
   bool printed_error_;
   bool trace_sync_io_;
   bool abort_on_uncaught_exception_;
+  bool emit_napi_warning_;
   size_t makecallback_cntr_;
   std::vector<double> destroy_ids_list_;
 

src/node.cc

@@ -193,9 +193,6 @@ unsigned int reverted = 0;
 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;
 
@@ -2650,27 +2647,22 @@ static 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),
-               "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);
+  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),
+             "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
@@ -3769,7 +3761,8 @@ static void PrintHelp() {
          "  --throw-deprecation        throw an exception on deprecations\n"
          "  --pending-deprecation      emit pending deprecation warnings\n"
          "  --no-warnings              silence all process warnings\n"
-         "  --napi-modules             load N-API modules\n"
+         "  --napi-modules             load N-API modules (no-op - option\n"
+         "                             kept for compatibility)\n"
          "  --abort-on-uncaught-exception\n"
          "                             aborting instead of exiting causes a\n"
          "                             core file to be generated for analysis\n"
@@ -4027,7 +4020,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) {
@@ -4665,11 +4658,6 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
 
   env.set_trace_sync_io(trace_sync_io);
 
-  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

@@ -841,28 +841,10 @@ void napi_module_register_cb(v8::Local<v8::Object> exports,
 
 }  // end of anonymous namespace
 
-#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'));

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')