From 0e10717e437eeb05e57d86e823777f418f00c478 Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Sun, 22 Oct 2017 12:16:48 -0700 Subject: [PATCH] build: runtime-deprecate requiring deps PR-URL: https://github.com/nodejs/node/pull/16392 Fixes: https://github.com/nodejs/node/pull/15566#discussion_r146111170 Reviewed-By: Refael Ackermann Reviewed-By: James M Snell Reviewed-By: Colin Ihrig Reviewed-By: Ben Noordhuis --- doc/api/deprecations.md | 31 ++++++++++++++++++ lib/internal/bootstrap_node.js | 17 ++++++++-- lib/internal/v8_prof_processor.js | 20 ++++++------ .../parallel/test-require-deps-deprecation.js | 32 +++++++++++++++++++ tools/js2c.py | 30 ++++++++++++++++- 5 files changed, 117 insertions(+), 13 deletions(-) create mode 100644 test/parallel/test-require-deps-deprecation.js diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index b8170f1e440309..6cd51421935f59 100644 --- 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. + +### 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 index 62b6911cbf9062..63020df5d3af6b 100644 --- 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 index 01b81c6ba56492..c255debc9ff35d 100644 --- 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 new file mode 100644 index 00000000000000..58dd47d148e321 --- /dev/null +++ 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 index f7951617d34064..5a97fdaecfa51e 100755 --- 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),