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.

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

Conversation

Projects
None yet
8 participants
@BridgeAR
Copy link
Member

commented Aug 31, 2017

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

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

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

Can you explain why da1af3d is no longer required?

@benjamingr
Copy link
Member

left a comment

Requesting changes so I don't forget this

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2017

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

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

Thanks, cc @bnoordhuis

@addaleax addaleax self-requested a review Aug 31, 2017

@Fishrock123
Copy link
Member

left a comment

little unsure why some bits were changed?

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

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Aug 31, 2017

Member

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

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 31, 2017

Author Member

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?

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

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Aug 31, 2017

Member

The previous commit here still seems valid...?

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 31, 2017

Author Member

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.

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

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

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Aug 31, 2017

Member

needs if (!addCommandLineAPI) return;?

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 31, 2017

Author Member

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

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

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Sep 1, 2017

Member

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

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Sep 1, 2017

Author Member

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

@jasnell jasnell requested a review from bnoordhuis Sep 1, 2017

@BridgeAR BridgeAR referenced this pull request Sep 2, 2017

Closed

lib: guard inspector console using process var #15008

2 of 2 tasks complete

@nodejs nodejs deleted a comment Sep 3, 2017

@nodejs nodejs deleted a comment Sep 3, 2017

@nodejs nodejs deleted a comment Sep 3, 2017

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2017

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2017

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Sep 15, 2017

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

@targos

This comment has been minimized.

Copy link
Member

commented Sep 18, 2017

LGTM if CI is happy

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2017

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Sep 19, 2017

Landed in 4bcb1c3

@BridgeAR BridgeAR closed this Sep 19, 2017

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

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

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

lib: refactor console startup
PR-URL: #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
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
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
Previously, console had to be compiled in case it was not available
but this is no longer necessary - remove it.

Refs: nodejs#15111

@apapirovski apapirovski referenced this pull request Oct 15, 2017

Closed

events: several perf related adjustments #16212

2 of 2 tasks complete

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

events: remove unnecessary console instantiation
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>

addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017

events: remove unnecessary console instantiation
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>

addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017

events: remove unnecessary console instantiation
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

This comment has been minimized.

Copy link
Member Author

commented Dec 11, 2017

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

@BridgeAR BridgeAR deleted the BridgeAR:refactor-console-bootstrap branch Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.