lib: refactor console bootstrap #15111

BridgeAR · 2017-08-31T03:04:39Z

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

benjamingr · 2017-08-31T07:41:08Z

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

Can you explain why da1af3d is no longer required?

benjamingr

Requesting changes so I don't forget this

BridgeAR · 2017-08-31T07:52:29Z

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

benjamingr · 2017-08-31T07:53:41Z

Thanks, cc @bnoordhuis

Fishrock123

little unsure why some bits were changed?

Fishrock123 · 2017-08-31T18:16:55Z

lib/internal/bootstrap_node.js

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


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?

Fishrock123 · 2017-08-31T18:17:20Z

lib/internal/bootstrap_node.js

    if (browserGlobals) {
-      // Instantiate eagerly in case the first call is under stack overflow
-      // conditions where instantiation doesn't work.


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.

Fishrock123 · 2017-08-31T18:18:31Z

lib/internal/bootstrap_node.js

@@ -312,62 +297,48 @@

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


needs if (!addCommandLineAPI) return;?

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

TimothyGu · 2017-09-01T00:21:40Z

lib/internal/bootstrap_node.js

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


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.

BridgeAR · 2017-09-08T03:51:27Z

@Fishrock123 @TimothyGu PTAL

BridgeAR · 2017-09-13T18:43:25Z

@nodejs/collaborators PTAL

BridgeAR · 2017-09-15T22:12:32Z

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

targos · 2017-09-18T11:09:00Z

LGTM if CI is happy

BridgeAR · 2017-09-19T20:56:49Z

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

BridgeAR · 2017-09-19T23:20:32Z

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>

BridgeAR · 2017-12-11T06:19:44Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: refactor console bootstrap #15111

lib: refactor console bootstrap #15111

BridgeAR commented Aug 31, 2017

benjamingr commented Aug 31, 2017

benjamingr left a comment

BridgeAR commented Aug 31, 2017

benjamingr commented Aug 31, 2017

Fishrock123 left a comment

Fishrock123 Aug 31, 2017

BridgeAR Aug 31, 2017

Fishrock123 Aug 31, 2017

BridgeAR Aug 31, 2017

Fishrock123 Aug 31, 2017

BridgeAR Aug 31, 2017

TimothyGu Sep 1, 2017

BridgeAR Sep 1, 2017

BridgeAR commented Sep 8, 2017

BridgeAR commented Sep 13, 2017

BridgeAR commented Sep 15, 2017 •

edited

Loading

targos commented Sep 18, 2017

BridgeAR commented Sep 19, 2017

BridgeAR commented Sep 19, 2017

BridgeAR commented Dec 11, 2017

lib: refactor console bootstrap #15111

lib: refactor console bootstrap #15111

Conversation

BridgeAR commented Aug 31, 2017

Checklist

Affected core subsystem(s)

benjamingr commented Aug 31, 2017

benjamingr left a comment

Choose a reason for hiding this comment

BridgeAR commented Aug 31, 2017

benjamingr commented Aug 31, 2017

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Sep 8, 2017

BridgeAR commented Sep 13, 2017

BridgeAR commented Sep 15, 2017 • edited Loading

targos commented Sep 18, 2017

BridgeAR commented Sep 19, 2017

BridgeAR commented Sep 19, 2017

BridgeAR commented Dec 11, 2017

BridgeAR commented Sep 15, 2017 •

edited

Loading