build: runtime-deprecate requiring deps

PR-URL: #16392 Fixes: #15566 (comment) Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
nodejs · Oct 22, 2017 · 0e10717e437eeb05e57d86e823777f418f00c478 · 0e10717
1 parent 14d24cc
commit 0e10717e437eeb05e57d86e823777f418f00c478
Unified Split

Showing with 117 additions and 13 deletions.

+31 −0 doc/api/deprecations.md

+15 −2 lib/internal/bootstrap_node.js

+10 −10 lib/internal/v8_prof_processor.js

+32 −0 test/parallel/test-require-deps-deprecation.js

+29 −1 tools/js2c.py
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
@@ -747,6 +747,37 @@ be set to `false` to disable ECDH entirely on the server only. This mode is
 deprecated in preparation for migrating to OpenSSL 1.1.0 and consistency with
 the client. Use the `ciphers` parameter instead.

+<a id="DEP0084"></a>
+### DEP0084: requiring bundled internal dependencies
+
+Type: Runtime
+
+Since Node.js versions 4.4.0 and 5.2.0, several modules only intended for
+internal usage are mistakenly exposed to user code through `require()`. These
+modules are:
+
+- `v8/tools/codemap`
+- `v8/tools/consarray`
+- `v8/tools/csvparser`
+- `v8/tools/logreader`
+- `v8/tools/profile_view`
+- `v8/tools/profile`
+- `v8/tools/SourceMap`
+- `v8/tools/splaytree`
+- `v8/tools/tickprocessor-driver`
+- `v8/tools/tickprocessor`
+- `node-inspect/lib/_inspect` (from 7.6.0)
+- `node-inspect/lib/internal/inspect_client` (from 7.6.0)
+- `node-inspect/lib/internal/inspect_repl` (from 7.6.0)
+
+The `v8/*` modules do not have any exports, and if not imported in a specific
+order would in fact throw errors. As such there are virtually no legitimate use
+cases for importing them through `require()`.
+
+On the other hand, `node-inspect` may be installed locally through a package
+manager, as it is published on the npm registry under the same name. No source
+code modification is necessary if that is done.
+

 [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
 [`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array
diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js
@@ -130,7 +130,7 @@

      // Start the debugger agent
      process.nextTick(function() {
-        NativeModule.require('node-inspect/lib/_inspect').start();
+        NativeModule.require('internal/deps/node-inspect/lib/_inspect').start();
      });

    } else if (process.profProcess) {
@@ -548,6 +548,16 @@
    return nativeModule.exports;
  };

+  NativeModule.requireForDeps = function(id) {
+    if (!NativeModule.exists(id) ||
+        // TODO(TimothyGu): remove when DEP0084 reaches end of life.
+        id.startsWith('node-inspect/') ||
+        id.startsWith('v8/')) {
+      id = `internal/deps/${id}`;
+    }
+    return NativeModule.require(id);
+  };
+
  NativeModule.getCached = function(id) {
    return NativeModule._cache[id];
  };
@@ -598,7 +608,10 @@
        lineOffset: 0,
        displayErrors: true
      });
-      fn(this.exports, NativeModule.require, this, internalBinding);
+      const requireFn = this.id.startsWith('internal/deps/') ?
+        NativeModule.requireForDeps :
+        NativeModule.require;
+      fn(this.exports, requireFn, this, internalBinding);

      this.loaded = true;
    } finally {
diff --git a/lib/internal/v8_prof_processor.js b/lib/internal/v8_prof_processor.js
@@ -1,16 +1,16 @@
 /* eslint-disable strict */
 const scriptFiles = [
  'internal/v8_prof_polyfill',
-  'v8/tools/splaytree',
-  'v8/tools/codemap',
-  'v8/tools/csvparser',
-  'v8/tools/consarray',
-  'v8/tools/profile',
-  'v8/tools/profile_view',
-  'v8/tools/logreader',
-  'v8/tools/tickprocessor',
-  'v8/tools/SourceMap',
-  'v8/tools/tickprocessor-driver'
+  'internal/deps/v8/tools/splaytree',
+  'internal/deps/v8/tools/codemap',
+  'internal/deps/v8/tools/csvparser',
+  'internal/deps/v8/tools/consarray',
+  'internal/deps/v8/tools/profile',
+  'internal/deps/v8/tools/profile_view',
+  'internal/deps/v8/tools/logreader',
+  'internal/deps/v8/tools/tickprocessor',
+  'internal/deps/v8/tools/SourceMap',
+  'internal/deps/v8/tools/tickprocessor-driver'
 ];
 var script = '';

diff --git a/test/parallel/test-require-deps-deprecation.js b/test/parallel/test-require-deps-deprecation.js
@@ -0,0 +1,32 @@
+'use strict';
+
+const common = require('../common');
+// The v8 modules when imported leak globals. Disable global check.
+common.globalCheck = false;
+
+const deprecatedModules = [
+  'node-inspect/lib/_inspect',
+  'node-inspect/lib/internal/inspect_client',
+  'node-inspect/lib/internal/inspect_repl',
+  'v8/tools/SourceMap',
+  'v8/tools/codemap',
+  'v8/tools/consarray',
+  'v8/tools/csvparser',
+  'v8/tools/logreader',
+  'v8/tools/profile',
+  'v8/tools/profile_view',
+  'v8/tools/splaytree',
+  'v8/tools/tickprocessor',
+  'v8/tools/tickprocessor-driver'
+];
+
+common.expectWarning('DeprecationWarning', deprecatedModules.map((m) => {
+  return `Requiring Node.js-bundled '${m}' module is deprecated. ` +
+         'Please install the necessary module locally.';
+}));
+
+for (const m of deprecatedModules) {
+  try {
+    require(m);
+  } catch (err) {}
+}
diff --git a/tools/js2c.py b/tools/js2c.py
@@ -218,6 +218,14 @@ def ReadMacros(lines):
                  {value}.ToStringChecked(env->isolate())).FromJust());
 """

+DEPRECATED_DEPS = """\
+'use strict';
+process.emitWarning(
+  'Requiring Node.js-bundled \\'{module}\\' module is deprecated. Please ' +
+  'install the necessary module locally.', 'DeprecationWarning', 'DEP0084');
+module.exports = require('internal/deps/{module}');
+"""
+

 def Render(var, data):
  # Treat non-ASCII as UTF-8 and convert it to UTF-16.
@@ -256,10 +264,19 @@ def JS2C(source, target):
    lines = ExpandConstants(lines, consts)
    lines = ExpandMacros(lines, macros)

+    deprecated_deps = None
+
    # On Windows, "./foo.bar" in the .gyp file is passed as "foo.bar"
    # so don't assume there is always a slash in the file path.
    if '/' in name or '\\' in name:
-      name = '/'.join(re.split('/|\\\\', name)[1:])
+      split = re.split('/|\\\\', name)
+      if split[0] == 'deps':
+        if split[1] == 'node-inspect' or split[1] == 'v8':
+          deprecated_deps = split[1:]
+        split = ['internal'] + split
+      else:
+        split = split[1:]
+      name = '/'.join(split)

    name = name.split('.', 1)[0]
    var = name.replace('-', '_').replace('/', '_')
@@ -270,6 +287,17 @@ def JS2C(source, target):
    definitions.append(Render(value, lines))
    initializers.append(INITIALIZER.format(key=key, value=value))

+    if deprecated_deps is not None:
+      name = '/'.join(deprecated_deps)
+      name = name.split('.', 1)[0]
+      var = name.replace('-', '_').replace('/', '_')
+      key = '%s_key' % var
+      value = '%s_value' % var
+
+      definitions.append(Render(key, name))
+      definitions.append(Render(value, DEPRECATED_DEPS.format(module=name)))
+      initializers.append(INITIALIZER.format(key=key, value=value))
+
  # Emit result
  output = open(str(target[0]), "w")
  output.write(TEMPLATE.format(definitions=''.join(definitions),