process: refactor the bootstrap mode branching for readability #24673

joyeecheung · 2018-11-27T14:52:01Z

This patch refactors the branches for choosing the mode to run
Node.js in internal/bootstrap/node.js. Instead of inlining the
decision making all in startup, we create a startExecution()
function which either detects and start the non-user-code mode,
or prepares for user code execution (worker setup, preloading modules)
and starts user code execution.
We use early returns when we decide the mode to run Node.js in for fewer
indentations and better readability.

This patch also adds a few comments about the command-line switches
and a few TODOs to remove underscore properties on process that
are mainly used for bootstrap mode branching. It also includes
a few other refactoring such as inlining functions/variables
that are not reused and removing the default argument of
evalScript for better clarity.

Checklist

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

nodejs-github-bot · 2018-11-27T14:52:03Z

@joyeecheung sadly an error occured when I tried to trigger a build :(

joyeecheung · 2018-11-27T14:53:13Z

test/message/core_line_numbers.out

@@ -12,4 +12,4 @@ RangeError: Invalid input
    at tryModuleLoad (internal/modules/cjs/loader.js:*:*)
    at Function.Module._load (internal/modules/cjs/loader.js:*:*)
    at Function.Module.runMain (internal/modules/cjs/loader.js:*:*)
-    at startup (internal/bootstrap/node.js:*:*)


These stack frames are truncated because we have a default Error.stackTraceLimit = 10. Makes me wonder again that we probably should implement #20505

i can put it on the list of things to add once #23926 is complete

joyeecheung · 2018-11-27T14:53:47Z

cc @devsnek @addaleax @nodejs/process

For ease of review please use https://github.com/nodejs/node/pull/24673/files?w=1 to skip the indentation changes

devsnek

looks pretty good, i'm a fan of the new function names

danbev · 2018-11-28T04:38:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18980/ ✔️

This patch refactors the branches for choosing the mode to run Node.js in `internal/bootstrap/node.js`. Instead of inlining the decision making all in `startup`, we create a `startExecution()` function which either detects and start the non-user-code mode, or prepares for user code execution (worker setup, preloading modules) and starts user code execution. We use early returns when we decide the mode to run Node.js in for fewer indentations and better readability. This patch also adds a few comments about the command-line switches and a few TODOs to remove underscore properties on `process` that are mainly used for bootstrap mode branching. It also includes a few other refactoring such as inlining functions/variables that are not reused and removing the default argument of `evalScript` for better clarity.

joyeecheung · 2018-11-29T12:04:17Z

Rebased for the recent arrow function changes in node.js.
CI: https://ci.nodejs.org/job/node-test-pull-request/19042/

joyeecheung · 2018-11-29T12:05:01Z

test/message/stdin_messages.out

@@ -9,7 +9,7 @@ SyntaxError: Strict mode code may not include a with statement
    at Object.<anonymous> ([stdin]-wrapper:*:*)
    at Module._compile (internal/modules/cjs/loader.js:*:*)
    at evalScript (internal/bootstrap/node.js:*:*)
-    at Socket.<anonymous> (internal/bootstrap/node.js:*:*)
+    at Socket.process.stdin.on (internal/bootstrap/node.js:*:*)


Fun fact: somehow this change and the arrow function change help V8 infer the name of the process stdin callbacks

addaleax

Nice!

Trott · 2018-11-29T16:11:14Z

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

Trott · 2018-11-29T19:23:29Z

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19062/

Trott · 2018-11-29T20:56:18Z

Landed in 7b8058a

This patch refactors the branches for choosing the mode to run Node.js in `internal/bootstrap/node.js`. Instead of inlining the decision making all in `startup`, we create a `startExecution()` function which either detects and start the non-user-code mode, or prepares for user code execution (worker setup, preloading modules) and starts user code execution. We use early returns when we decide the mode to run Node.js in for fewer indentations and better readability. This patch also adds a few comments about the command-line switches and a few TODOs to remove underscore properties on `process` that are mainly used for bootstrap mode branching. It also includes a few other refactoring such as inlining functions/variables that are not reused and removing the default argument of `evalScript` for better clarity. PR-URL: nodejs#24673 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

This patch refactors the branches for choosing the mode to run Node.js in `internal/bootstrap/node.js`. Instead of inlining the decision making all in `startup`, we create a `startExecution()` function which either detects and start the non-user-code mode, or prepares for user code execution (worker setup, preloading modules) and starts user code execution. We use early returns when we decide the mode to run Node.js in for fewer indentations and better readability. This patch also adds a few comments about the command-line switches and a few TODOs to remove underscore properties on `process` that are mainly used for bootstrap mode branching. It also includes a few other refactoring such as inlining functions/variables that are not reused and removing the default argument of `evalScript` for better clarity. PR-URL: #24673 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

This patch refactors the branches for choosing the mode to run Node.js in `internal/bootstrap/node.js`. Instead of inlining the decision making all in `startup`, we create a `startExecution()` function which either detects and start the non-user-code mode, or prepares for user code execution (worker setup, preloading modules) and starts user code execution. We use early returns when we decide the mode to run Node.js in for fewer indentations and better readability. This patch also adds a few comments about the command-line switches and a few TODOs to remove underscore properties on `process` that are mainly used for bootstrap mode branching. It also includes a few other refactoring such as inlining functions/variables that are not reused and removing the default argument of `evalScript` for better clarity. PR-URL: nodejs#24673 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

joyeecheung commented Nov 27, 2018

View reviewed changes

devsnek approved these changes Nov 27, 2018

View reviewed changes

danbev approved these changes Nov 28, 2018

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2018

joyeecheung force-pushed the refactor-bootstrap branch from 1f141d0 to 0a7f427 Compare November 29, 2018 12:04

joyeecheung commented Nov 29, 2018

View reviewed changes

addaleax approved these changes Nov 29, 2018

View reviewed changes

cjihrig approved these changes Nov 29, 2018

View reviewed changes

Trott closed this Nov 29, 2018

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

jdalton mentioned this pull request Feb 6, 2019

Node 11.4.0+ fails to resolve index.js when combined with -r and --inspect-brk #25967

Closed

BethGriggs added the dont-land-on-v10.x label Jun 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

process: refactor the bootstrap mode branching for readability #24673

process: refactor the bootstrap mode branching for readability #24673

joyeecheung commented Nov 27, 2018

nodejs-github-bot commented Nov 27, 2018

joyeecheung Nov 27, 2018

devsnek Nov 27, 2018

joyeecheung commented Nov 27, 2018 •

edited

Loading

devsnek left a comment

danbev commented Nov 28, 2018 •

edited by Trott

Loading

joyeecheung commented Nov 29, 2018

joyeecheung Nov 29, 2018

addaleax left a comment

Trott commented Nov 29, 2018

Trott commented Nov 29, 2018

Trott commented Nov 29, 2018

process: refactor the bootstrap mode branching for readability #24673

process: refactor the bootstrap mode branching for readability #24673

Conversation

joyeecheung commented Nov 27, 2018

Checklist

nodejs-github-bot commented Nov 27, 2018

joyeecheung Nov 27, 2018

Choose a reason for hiding this comment

devsnek Nov 27, 2018

Choose a reason for hiding this comment

joyeecheung commented Nov 27, 2018 • edited Loading

devsnek left a comment

Choose a reason for hiding this comment

danbev commented Nov 28, 2018 • edited by Trott Loading

joyeecheung commented Nov 29, 2018

joyeecheung Nov 29, 2018

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

Trott commented Nov 29, 2018

Trott commented Nov 29, 2018

Trott commented Nov 29, 2018

joyeecheung commented Nov 27, 2018 •

edited

Loading

danbev commented Nov 28, 2018 •

edited by Trott

Loading