From f517b03c8adbb32b8b5abc48fcf4d548d4ecec1e Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 29 Nov 2018 06:14:30 +0800 Subject: [PATCH 1/3] lib: move lib/console.js to lib/internal/console/constructor.js This is a broken commit: it's here so that git interpret this as a file move and preserve most of the history of the Console constructor. --- lib/{console.js => internal/console/constructor.js} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename lib/{console.js => internal/console/constructor.js} (100%) diff --git a/lib/console.js b/lib/internal/console/constructor.js similarity index 100% rename from lib/console.js rename to lib/internal/console/constructor.js From f0962d4c1aa48b390b62c8ca8cafaf87ae1dbfd0 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 29 Nov 2018 06:17:57 +0800 Subject: [PATCH 2/3] console: split console into global.js and constructor.js Since we do not actually use the Console constructor to instantiate the global console, move the two piece of code into two different JS files for clarity, and make console.js a mere re-export of the global console. The hope is to make the global console, a namespace, more web-compatible while keeping the Console constructor available for backwards compatibility. --- lib/console.js | 24 +++++++++ lib/internal/console/constructor.js | 69 +++---------------------- lib/internal/console/global.js | 44 ++++++++++++++++ node.gyp | 2 + test/parallel/test-bootstrap-modules.js | 2 +- 5 files changed, 79 insertions(+), 62 deletions(-) create mode 100644 lib/console.js create mode 100644 lib/internal/console/global.js diff --git a/lib/console.js b/lib/console.js new file mode 100644 index 00000000000000..85a89ecc990b8b --- /dev/null +++ b/lib/console.js @@ -0,0 +1,24 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +'use strict'; + +module.exports = require('internal/console/global'); diff --git a/lib/internal/console/constructor.js b/lib/internal/console/constructor.js index ebd098e9affdff..f607bf4648bbff 100644 --- a/lib/internal/console/constructor.js +++ b/lib/internal/console/constructor.js @@ -1,26 +1,8 @@ -// Copyright Joyent, Inc. and other Node contributors. -// -// Permission is hereby granted, free of charge, to any person obtaining a -// copy of this software and associated documentation files (the -// "Software"), to deal in the Software without restriction, including -// without limitation the rights to use, copy, modify, merge, publish, -// distribute, sublicense, and/or sell copies of the Software, and to permit -// persons to whom the Software is furnished to do so, subject to the -// following conditions: -// -// The above copyright notice and this permission notice shall be included -// in all copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS -// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF -// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN -// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, -// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR -// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE -// USE OR OTHER DEALINGS IN THE SOFTWARE. - 'use strict'; +// The Console constructor is not actually used to construct the global +// console. It's exported for backwards compatibility. + const { trace } = internalBinding('trace_events'); const { isStackOverflowError, @@ -72,8 +54,6 @@ const kBindStreamsLazy = Symbol('kBindStreamsLazy'); const kUseStdout = Symbol('kUseStdout'); const kUseStderr = Symbol('kUseStderr'); -// This constructor is not used to construct the global console. -// It's exported for backwards compatibility. function Console(options /* or: stdout, stderr, ignoreErrors = true */) { // We have to test new.target here to see if this function is called // with new, because we need to define a custom instanceof to accommodate @@ -531,41 +511,8 @@ Console.prototype.table = function(tabularData, properties) { function noop() {} -// See https://console.spec.whatwg.org/#console-namespace -// > For historical web-compatibility reasons, the namespace object -// > for console must have as its [[Prototype]] an empty object, -// > created as if by ObjectCreate(%ObjectPrototype%), -// > instead of %ObjectPrototype%. - -// Since in Node.js, the Console constructor has been exposed through -// require('console'), we need to keep the Console constructor but -// we cannot actually use `new Console` to construct the global console. -// Therefore, the console.Console.prototype is not -// in the global console prototype chain anymore. - -// TODO(joyeecheung): -// - Move the Console constructor into internal/console.js -// - Move the global console creation code along with the inspector console -// wrapping code in internal/bootstrap/node.js into a separate file. -// - Make this file a simple re-export of those two files. -const globalConsole = Object.create({}); - -// Since Console is not on the prototype chain of the global console, -// the symbol properties on Console.prototype have to be looked up from -// the global console itself. In addition, we need to make the global -// console a namespace by binding the console methods directly onto -// the global console with the receiver fixed. -for (const prop of Reflect.ownKeys(Console.prototype)) { - if (prop === 'constructor') { continue; } - const desc = Reflect.getOwnPropertyDescriptor(Console.prototype, prop); - if (typeof desc.value === 'function') { // fix the receiver - desc.value = desc.value.bind(globalConsole); - } - Reflect.defineProperty(globalConsole, prop, desc); -} - -globalConsole[kBindStreamsLazy](process); -globalConsole[kBindProperties](true, 'auto'); - -module.exports = globalConsole; -module.exports.Console = Console; +module.exports = { + Console, + kBindStreamsLazy, + kBindProperties +}; diff --git a/lib/internal/console/global.js b/lib/internal/console/global.js new file mode 100644 index 00000000000000..614941eba6da2d --- /dev/null +++ b/lib/internal/console/global.js @@ -0,0 +1,44 @@ +'use strict'; + +// See https://console.spec.whatwg.org/#console-namespace +// > For historical web-compatibility reasons, the namespace object +// > for console must have as its [[Prototype]] an empty object, +// > created as if by ObjectCreate(%ObjectPrototype%), +// > instead of %ObjectPrototype%. + +// Since in Node.js, the Console constructor has been exposed through +// require('console'), we need to keep the Console constructor but +// we cannot actually use `new Console` to construct the global console. +// Therefore, the console.Console.prototype is not +// in the global console prototype chain anymore. + +const { + Console, + kBindStreamsLazy, + kBindProperties +} = require('internal/console/constructor'); + +const globalConsole = Object.create({}); + +// Since Console is not on the prototype chain of the global console, +// the symbol properties on Console.prototype have to be looked up from +// the global console itself. In addition, we need to make the global +// console a namespace by binding the console methods directly onto +// the global console with the receiver fixed. +for (const prop of Reflect.ownKeys(Console.prototype)) { + if (prop === 'constructor') { continue; } + const desc = Reflect.getOwnPropertyDescriptor(Console.prototype, prop); + if (typeof desc.value === 'function') { // fix the receiver + desc.value = desc.value.bind(globalConsole); + } + Reflect.defineProperty(globalConsole, prop, desc); +} + +globalConsole[kBindStreamsLazy](process); +globalConsole[kBindProperties](true, 'auto'); + +// This is a legacy feature - the Console constructor is exposed on +// the global console instance. +globalConsole.Console = Console; + +module.exports = globalConsole; diff --git a/node.gyp b/node.gyp index 3cbbe61e575ca3..b506577381499f 100644 --- a/node.gyp +++ b/node.gyp @@ -95,6 +95,8 @@ 'lib/internal/cluster/shared_handle.js', 'lib/internal/cluster/utils.js', 'lib/internal/cluster/worker.js', + 'lib/internal/console/constructor.js', + 'lib/internal/console/global.js', 'lib/internal/crypto/certificate.js', 'lib/internal/crypto/cipher.js', 'lib/internal/crypto/diffiehellman.js', diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 272dec40060624..8f83194cbb1a52 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -9,7 +9,7 @@ const common = require('../common'); const assert = require('assert'); const isMainThread = common.isMainThread; -const kMaxModuleCount = isMainThread ? 56 : 78; +const kMaxModuleCount = isMainThread ? 58 : 80; assert(list.length <= kMaxModuleCount, `Total length: ${list.length}\n` + list.join('\n') From 24f7c80370e12adc23e782d35eba60d34a6edab1 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 29 Nov 2018 06:24:14 +0800 Subject: [PATCH 3/3] console: move the inspector console wrapping in a separate file Move the wrapping of the inspector console in a separate file for clarity. In addition, save the original console from the VM explicitly via an exported property `require('internal/console/inspector').consoleFromVM` that `require('inspector').console` can alias to it later, instead of hanging the original console onto `per_thread.js` during bootstrap and counting on that `per_thread.js` only gets evaluated once and gets cached. --- lib/inspector.js | 5 ++- lib/internal/bootstrap/node.js | 68 +++++++------------------------ lib/internal/console/inspector.js | 53 ++++++++++++++++++++++++ lib/internal/util.js | 12 ++++++ node.gyp | 1 + 5 files changed, 84 insertions(+), 55 deletions(-) create mode 100644 lib/internal/console/inspector.js diff --git a/lib/inspector.js b/lib/inspector.js index 6988eccf82c9ef..14ea01e6adc6c7 100644 --- a/lib/inspector.js +++ b/lib/inspector.js @@ -12,7 +12,6 @@ const { const { validateString } = require('internal/validators'); const util = require('util'); const { Connection, open, url } = process.binding('inspector'); -const { originalConsole } = require('internal/process/per_thread'); if (!Connection) throw new ERR_INSPECTOR_NOT_AVAILABLE(); @@ -103,6 +102,8 @@ module.exports = { open: (port, host, wait) => open(port, host, !!wait), close: process._debugEnd, url: url, - console: originalConsole, + // This is dynamically added during bootstrap, + // where the console from the VM is still available + console: require('internal/console/inspector').consoleFromVM, Session }; diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 21544bceb463d1..b081e403082980 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -126,8 +126,6 @@ const browserGlobals = !process._noBrowserGlobals; if (browserGlobals) { - // we are setting this here to forward it to the inspector later - perThreadSetup.originalConsole = global.console; setupGlobalTimeouts(); setupGlobalConsole(); setupGlobalURL(); @@ -438,16 +436,25 @@ } function setupGlobalConsole() { - const originalConsole = global.console; - // Setup Node.js global.console. - const wrappedConsole = NativeModule.require('console'); + const consoleFromVM = global.console; + const consoleFromNode = + NativeModule.require('internal/console/global'); + // Override global console from the one provided by the VM + // to the one implemented by Node.js Object.defineProperty(global, 'console', { configurable: true, enumerable: false, - value: wrappedConsole, + value: consoleFromNode, writable: true }); - setupInspector(originalConsole, wrappedConsole); + // TODO(joyeecheung): can we skip this if inspector is not active? + if (process.config.variables.v8_enable_inspector) { + const inspector = + NativeModule.require('internal/console/inspector'); + inspector.addInspectorApis(consoleFromNode, consoleFromVM); + // This will be exposed by `require('inspector').console` later. + inspector.consoleFromVM = consoleFromVM; + } } function setupGlobalURL() { @@ -520,41 +527,6 @@ NativeModule.require('internal/domexception'); } - function setupInspector(originalConsole, wrappedConsole) { - if (!process.config.variables.v8_enable_inspector) { - return; - } - const CJSModule = NativeModule.require('internal/modules/cjs/loader'); - const { addCommandLineAPI, consoleCall } = process.binding('inspector'); - // Setup inspector command line API. - const { makeRequireFunction } = - NativeModule.require('internal/modules/cjs/helpers'); - const path = NativeModule.require('path'); - const cwd = tryGetCwd(path); - - const consoleAPIModule = new CJSModule(''); - consoleAPIModule.paths = - CJSModule._nodeModulePaths(cwd).concat(CJSModule.globalPaths); - addCommandLineAPI('require', makeRequireFunction(consoleAPIModule)); - const config = {}; - for (const key of Object.keys(wrappedConsole)) { - if (!originalConsole.hasOwnProperty(key)) - continue; - // If global console has the same method as inspector console, - // then wrap these two methods into one. Native wrapper will preserve - // the original stack. - wrappedConsole[key] = consoleCall.bind(wrappedConsole, - originalConsole[key], - wrappedConsole[key], - config); - } - for (const key of Object.keys(originalConsole)) { - if (wrappedConsole.hasOwnProperty(key)) - continue; - wrappedConsole[key] = originalConsole[key]; - } - } - function noop() {} function setupProcessFatal() { @@ -633,17 +605,6 @@ } } - function tryGetCwd(path) { - try { - return process.cwd(); - } catch { - // getcwd(3) can fail if the current working directory has been deleted. - // Fall back to the directory name of the (absolute) executable path. - // It's not really correct but what are the alternatives? - return path.dirname(process.execPath); - } - } - function wrapForBreakOnFirstLine(source) { if (!process._breakFirstLine) return source; @@ -654,6 +615,7 @@ function evalScript(name, body = wrapForBreakOnFirstLine(process._eval)) { const CJSModule = NativeModule.require('internal/modules/cjs/loader'); const path = NativeModule.require('path'); + const { tryGetCwd } = NativeModule.require('internal/util'); const cwd = tryGetCwd(path); const module = new CJSModule(name); diff --git a/lib/internal/console/inspector.js b/lib/internal/console/inspector.js new file mode 100644 index 00000000000000..5e04289be9ae6a --- /dev/null +++ b/lib/internal/console/inspector.js @@ -0,0 +1,53 @@ +'use strict'; + +const path = require('path'); +const CJSModule = require('internal/modules/cjs/loader'); +const { makeRequireFunction } = require('internal/modules/cjs/helpers'); +const { tryGetCwd } = require('internal/util'); +const { addCommandLineAPI, consoleCall } = process.binding('inspector'); + +// Wrap a console implemented by Node.js with features from the VM inspector +function addInspectorApis(consoleFromNode, consoleFromVM) { + // Setup inspector command line API. + const cwd = tryGetCwd(path); + const consoleAPIModule = new CJSModule(''); + consoleAPIModule.paths = + CJSModule._nodeModulePaths(cwd).concat(CJSModule.globalPaths); + addCommandLineAPI('require', makeRequireFunction(consoleAPIModule)); + const config = {}; + + // If global console has the same method as inspector console, + // then wrap these two methods into one. Native wrapper will preserve + // the original stack. + for (const key of Object.keys(consoleFromNode)) { + if (!consoleFromVM.hasOwnProperty(key)) + continue; + consoleFromNode[key] = consoleCall.bind(consoleFromNode, + consoleFromVM[key], + consoleFromNode[key], + config); + } + + // Add additional console APIs from the inspector + for (const key of Object.keys(consoleFromVM)) { + if (consoleFromNode.hasOwnProperty(key)) + continue; + consoleFromNode[key] = consoleFromVM[key]; + } +} + +module.exports = { + addInspectorApis +}; + +// Stores the console from VM, should be set during bootstrap. +let consoleFromVM; + +Object.defineProperty(module.exports, 'consoleFromVM', { + get() { + return consoleFromVM; + }, + set(val) { + consoleFromVM = val; + } +}); diff --git a/lib/internal/util.js b/lib/internal/util.js index 3524b9e1d62112..414d037a298ca6 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -373,6 +373,17 @@ function once(callback) { }; } +function tryGetCwd(path) { + try { + return process.cwd(); + } catch { + // getcwd(3) can fail if the current working directory has been deleted. + // Fall back to the directory name of the (absolute) executable path. + // It's not really correct but what are the alternatives? + return path.dirname(process.execPath); + } +} + module.exports = { assertCrypto, cachedResult, @@ -392,6 +403,7 @@ module.exports = { once, promisify, spliceOne, + tryGetCwd, removeColors, // Symbol used to customize promisify conversion diff --git a/node.gyp b/node.gyp index b506577381499f..661727d9d58729 100644 --- a/node.gyp +++ b/node.gyp @@ -97,6 +97,7 @@ 'lib/internal/cluster/worker.js', 'lib/internal/console/constructor.js', 'lib/internal/console/global.js', + 'lib/internal/console/inspector.js', 'lib/internal/crypto/certificate.js', 'lib/internal/crypto/cipher.js', 'lib/internal/crypto/diffiehellman.js',