Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: refactor console bootstrap #15111

Closed
wants to merge 3 commits into from
Closed

lib: refactor console bootstrap #15111

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a7ddc2d
lib: refactor console startup
BridgeAR Aug 29, 2017
0da4d6d
[squash] address comments
BridgeAR Aug 31, 2017
5db3a1b
[squash] use individual inspectorSetup
BridgeAR Sep 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 31 additions & 61 deletions lib/internal/bootstrap_node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,19 @@
setupProcessICUVersions();

setupGlobalVariables();

const _process = NativeModule.require('internal/process');
_process.setupConfig(NativeModule._source);
NativeModule.require('internal/process/warning').setup();
NativeModule.require('internal/process/next_tick').setup();
NativeModule.require('internal/process/stdio').setup();

const browserGlobals = !process._noBrowserGlobals;
if (browserGlobals) {
setupGlobalTimeouts();
setupGlobalConsole();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is desirable to load the console first for debugging reasons.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that is a bit tricky if we want to instantiate it eagerly without the get call to do the Instantiation because internal/process/stdio has to be loaded first. Would it be fine to move the necessary part up that load stdio and keep the eager instantiation?


const _process = NativeModule.require('internal/process');
const perf = process.binding('performance');
const {
NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE,
Expand All @@ -50,26 +56,6 @@
_process.setup_performance();
_process.setup_cpuUsage();
_process.setupMemoryUsage();
_process.setupConfig(NativeModule._source);
NativeModule.require('internal/process/warning').setup();
NativeModule.require('internal/process/next_tick').setup();
NativeModule.require('internal/process/stdio').setup();
if (browserGlobals) {
// Instantiate eagerly in case the first call is under stack overflow
// conditions where instantiation doesn't work.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous commit here still seems valid...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat yes, but it did not fully work as anticipated and therefore I think it is fine to remove the comment. I can add the comment back in if you like though.

const console = global.console;
console.assert;
console.clear;
console.count;
console.countReset;
console.dir;
console.error;
console.log;
console.time;
console.timeEnd;
console.trace;
console.warn;
}
_process.setupKillAndExit();
_process.setupSignalHandlers();
if (global.__coverage__)
Expand Down Expand Up @@ -312,62 +298,46 @@

function setupGlobalConsole() {
const originalConsole = global.console;
let console;
const Module = NativeModule.require('module');
// Setup Node.js global.console
const wrappedConsole = NativeModule.require('console');
Object.defineProperty(global, 'console', {
configurable: true,
enumerable: true,
get: function() {
if (!console) {
console = (originalConsole === undefined) ?
NativeModule.require('console') :
installInspectorConsole(originalConsole);
}
return console;
get() {
return wrappedConsole;
}
});
setupInspectorCommandLineAPI();
}

function installInspectorConsole(globalConsole) {
const wrappedConsole = NativeModule.require('console');
const inspector = process.binding('inspector');
if (!inspector.consoleCall) {
return wrappedConsole;
// Setup inspector command line API
const { addCommandLineAPI, consoleCall } = process.binding('inspector');
if (!consoleCall) {
return;
}
const { makeRequireFunction } = NativeModule.require('internal/module');
const path = NativeModule.require('path');
const cwd = tryGetCwd(path);

const consoleAPIModule = new Module('<inspector console>');
consoleAPIModule.paths =
Module._nodeModulePaths(cwd).concat(Module.globalPaths);
addCommandLineAPI('require', makeRequireFunction(consoleAPIModule));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is frankly unrelated to console instantiation. I'd prefer it to be moved out of console bootstrapping.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I combined them because it was the only function calling the inspector. But I split them and the inspector part is now separated.

const config = {};
for (const key of Object.keys(wrappedConsole)) {
if (!globalConsole.hasOwnProperty(key))
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] = inspector.consoleCall.bind(wrappedConsole,
globalConsole[key],
wrappedConsole[key],
config);
wrappedConsole[key] = consoleCall.bind(wrappedConsole,
originalConsole[key],
wrappedConsole[key],
config);
}
for (const key of Object.keys(globalConsole)) {
for (const key of Object.keys(originalConsole)) {
if (wrappedConsole.hasOwnProperty(key))
continue;
wrappedConsole[key] = globalConsole[key];
wrappedConsole[key] = originalConsole[key];
}
return wrappedConsole;
}

function setupInspectorCommandLineAPI() {
const { addCommandLineAPI } = process.binding('inspector');
if (!addCommandLineAPI) return;

const Module = NativeModule.require('module');
const { makeRequireFunction } = NativeModule.require('internal/module');
const path = NativeModule.require('path');
const cwd = tryGetCwd(path);

const consoleAPIModule = new Module('<inspector console>');
consoleAPIModule.paths =
Module._nodeModulePaths(cwd).concat(Module.globalPaths);

addCommandLineAPI('require', makeRequireFunction(consoleAPIModule));
}

function setupProcessFatal() {
Expand Down