lib: refactor console bootstrap by BridgeAR · Pull Request #15111 · nodejs/node · GitHub

Console setup is currently more complicated then it has to be. I refactored it to be more straight forward.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

lib, test

has nodejs/help#778 been fixed otherwise in the last 19 days?

Can you explain why da1af3d is no longer required?

Requesting changes so I don't forget this

For question 1 please look at #15110 (it was only seemingly fixed but not really fixed).

The initiation of global.console is not necessary anymore because that was originally done in the get of global.console and now it is done right away. That is why the test added by da1af3d continues to "work".

addressed

Thanks, cc @bnoordhuis

little unsure why some bits were changed?

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

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?

The previous commit here still seems valid...?

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.

needs if (!addCommandLineAPI) return;?

Oh, you are right, this part should be moved below the if (!consoleCall) statement. That should be sufficient.

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

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

@Fishrock123 @TimothyGu PTAL

@nodejs/collaborators PTAL

Since this should actually also fix a bug (see #15008 (comment)) it would be nice to get some reviews.

LGTM if CI is happy

CI https://ci.nodejs.org/job/node-test-pull-request/10143/

Landed in 4bcb1c3

PR-URL: #15111 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: nodejs/node#15111 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Previously, console had to be compiled in case it was not available but this is no longer necessary - remove it. Refs: nodejs#15111

Previously, console had to be compiled in case it was not available but this is no longer necessary - remove it. PR-URL: nodejs#16212 Refs: nodejs#15111 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Previously, console had to be compiled in case it was not available but this is no longer necessary - remove it. PR-URL: nodejs/node#16212 Refs: nodejs/node#15111 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

I do not see much benefit to backport this, so I changed the label accordingly.

lib: refactor console startup

a7ddc2d

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 31, 2017

benjamingr previously requested changes Aug 31, 2017

View reviewed changes

addaleax self-requested a review August 31, 2017 13:25

Fishrock123 reviewed Aug 31, 2017

View reviewed changes

[squash] address comments

0da4d6d

TimothyGu reviewed Sep 1, 2017

View reviewed changes

[squash] use individual inspectorSetup

5db3a1b

jasnell requested a review from bnoordhuis September 1, 2017 17:42

BridgeAR mentioned this pull request Sep 2, 2017

lib: guard inspector console using process var #15008

Closed

2 tasks

nodejs deleted a comment Sep 3, 2017

jasnell approved these changes Sep 18, 2017

View reviewed changes

BridgeAR closed this Sep 19, 2017

BridgeAR added a commit that referenced this pull request Sep 19, 2017

lib: refactor console startup

4bcb1c3

PR-URL: #15111 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

jasnell pushed a commit that referenced this pull request Sep 20, 2017

lib: refactor console startup

8ce0e9a

PR-URL: #15111 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017

lib: refactor console startup

9f3f5ef

PR-URL: nodejs/node#15111 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017

lib: refactor console startup

6bf1cd9

PR-URL: nodejs/node#15111 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

apapirovski added a commit to apapirovski/node that referenced this pull request Oct 15, 2017

events: remove unnecessary console instantiation

75571fb

Previously, console had to be compiled in case it was not available but this is no longer necessary - remove it. Refs: nodejs#15111

apapirovski mentioned this pull request Oct 15, 2017

events: several perf related adjustments #16212

Closed

2 tasks

MylesBorins added the lts-watch-v6.x label Oct 17, 2017

BridgeAR added dont-land-on-v6.x and removed lts-watch-v6.x labels Dec 11, 2017

BridgeAR deleted the refactor-console-bootstrap branch April 1, 2019 23:37

+                  const browserGlobals = !process._noBrowserGlobals;
                   if (browserGlobals) {
-                    // Instantiate eagerly in case the first call is under stack overflow
-                    // conditions where instantiation doesn't work.

                   const originalConsole = global.console;
-                  let console;
+                  // Setup inspector command line API
+                  const { addCommandLineAPI, consoleCall } = process.binding('inspector');

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

-                  if (browserGlobals) {
-                    setupGlobalTimeouts();
-                    setupGlobalConsole();
-                  }

lib: refactor console bootstrap by BridgeAR · Pull Request #15111 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: refactor console bootstrap - #15111

lib: refactor console bootstrap#15111BridgeAR wants to merge 3 commits intonodejs:masternodejs/node:masterfrom BridgeAR:refactor-console-bootstrapBridgeAR/node:refactor-console-bootstrapCopy head branch name to clipboard

Conversation

BridgeAR commented Aug 31, 2017

Checklist

Affected core subsystem(s)

Uh oh!

benjamingr commented Aug 31, 2017

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 31, 2017

Uh oh!

benjamingr commented Aug 31, 2017

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 8, 2017

Uh oh!

BridgeAR commented Sep 13, 2017

Uh oh!

BridgeAR commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Sep 18, 2017

Uh oh!

BridgeAR commented Sep 19, 2017

Uh oh!

BridgeAR commented Sep 19, 2017

Uh oh!

BridgeAR commented Dec 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lib: refactor console bootstrap#15111
BridgeAR wants to merge 3 commits into
nodejs:masterfrom
BridgeAR:refactor-console-bootstrap

BridgeAR commented Sep 15, 2017 •
edited
Loading