refactor src/node.js into internal files by Fishrock123 · Pull Request #5103 · nodejs/node · GitHub

Ok so, this is definitely a bit nuts. Just a bit. Well, the second commit that is.

I figure what this costs us in the short term will be made up for by ease of contribution in the future.

commit 1: moves promise setup, stdio, and nextTick out of node.js
commit 2: move src/node.js to lib/internal/node.js

This PR is mostly about the first set of changes. Although I'd also like to do the second. src/node.js is very big and it's fairly hard to figure out what you all might need to change for any specific patch.

CI: https://ci.nodejs.org/job/node-test-pull-request/1562/
R=@nodejs/ctc

I dunno how to get around some of this dependancy injection for now... Maybe add these to process and then remove them after boot?

(fixed linting)

Why do you need NativeModule? I think you can just use require at this point

@vkurchatkin We don't want this in the regular module cache.

what do you mean? require and NativeModule.require are literally the same thing in built-in modules: https://github.com/nodejs/node/blob/master/src/node.js#L1004

Ooooh good catch

Very interesting! This should go a long way towards clearing things up. It might be worthwhile to CC folks from NW.js and electron to make sure this is compatible with their projects, but if so I'm generally in favor of this.

cc @zcbenz / @rogerwang ^^^

(Looks like my previous comment got squashed)

I dunno how to get around some of this dependancy injection for now... Maybe add these to process and then remove them after boot?

Thanks for CCing @chrisdickinson @Fishrock123 .

It's fine for NW.js.

Rebased on master.

I have work that will be sitting ontop of this since it's easier to tell what needs modification, and as such I'd sorta like to get this merged.

cc @trevnorris

could be made const.

EDIT: nope. sorry.

going to add the ctc meeting tag. only b/c it relocates so much code, would like a decisive ok from everyone.

sigh I should have added it today. Already CC'd everyone though with almost no response?

Sorry, saw the ctc mention just hadn't had any time to review. Will look
through tomorrow.
On Feb 10, 2016 5:27 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

sigh I should have added it today. Already CC'd everyone though with
almost no response?

—
Reply to this email directly or view it on GitHub
#5103 (comment).

And before anyone asks, yes this is totally just moving some code around, that's the point haha. :P

At a high level this LGTM if CI is green.
I'm going to go out on a limb and mark this semver-major until we can be certain it doesn't break anything. That may be a bit strong, of course, but given how core this part of the code is it's best to be conservative. The tag can be downgraded if we're sure nothing breaks because of this :-)

Could you point to where you think it could be semver-major?

Do addons need the file location? I've already heard from two embedders (nw.js and N|Solid) that this doesn't cause significant issues either.

As I said, I'm just being conservative. It shouldn't break anything but it's worth being cautious.

One impact this will have is changing the stack trace on certain errors, e.g:

bash-3.2$ node ~/tmp/test.js
events.js:154
      throw er; // Unhandled 'error' event
      ^
Error: foo
    at Object.<anonymous> (/Users/james/tmp/test.js:5:17)
    at Module._compile (module.js:413:34)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Function.Module.runMain (module.js:447:10)
    at startup (node.js:139:18)
    at node.js:999:3
bash-3.2$

While that shouldn't have an impact, I think it's at least worth being cautious.

I've kicked off a CITGM run here: https://ci.nodejs.org/job/thealphanerd-smoker/66/

One impact this will have is changing the stack trace on certain errors, e.g:

This is not so terribly uncommon and I definitely do not think we guarantee stacktraces. That's like guaranteeing lib/internal APIs.

See: https://github.com/nodejs/node/commits/master/test/message -- 8830797 modifies the same traces but is scheduled for LTS.

Grrr... CITGM smoke testing is currently broken due to the npm/graceful-fs issue on master.
As I said, the change LGTM I'd just prefer to be very conservative with this part of the code and be extra certain. I'll drop the semver-major but I'm going to add a don't-land-in-v4.x until it's been out in a stable release for a while.

Also see @rvagg's comment here: #5092 (comment) ... so I'm not alone in my caution on this.

The one failure appears to be unrelated. Land it!

PR-URL: nodejs#5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

landed, thanks everyone!

@Fishrock123 ... woot!

Clarify comments re invoking bootstrap_node.js. Fix filename to bootstrap_node.js per nodejs#5103. Fix tests `node.js` -> `bootstrap_node.js` Fix comment on why we check the loop again before exiting. `context-inl.h` -> `env-inl.h`

PR-URL: nodejs#5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Conflicts: node.gyp src/node.js

PR-URL: nodejs#5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 added process Issues and PRs related to the process subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 5, 2016

Fishrock123 reviewed Feb 5, 2016
View reviewed changes

Fishrock123 force-pushed the cleanup-node.js branch from 2e216f1 to 0d88fc8 Compare February 5, 2016 18:43

vkurchatkin reviewed Feb 5, 2016
View reviewed changes

Fishrock123 reviewed Feb 6, 2016
View reviewed changes

Fishrock123 force-pushed the cleanup-node.js branch 2 times, most recently from e5888b1 to 65b9481 Compare February 6, 2016 16:34

Fishrock123 force-pushed the cleanup-node.js branch from 65b9481 to bf99893 Compare February 10, 2016 16:17

trevnorris reviewed Feb 11, 2016
View reviewed changes

trevnorris added the ctc-agenda label Feb 11, 2016

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 11, 2016

jasnell added dont-land-on-v4.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 11, 2016

Fishrock123 added 3 commits March 22, 2016 19:20

lib,src: refactor src/node.js into internal files

015cef2

PR-URL: nodejs#5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

lib,src: move src/node.js to lib/internal/node.js

4bf2aca

PR-URL: nodejs#5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

lib: rename /node.js to /bootstrap_node.js

ec6af31

PR-URL: nodejs#5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 force-pushed the cleanup-node.js branch from 2edd8f1 to ec6af31 Compare March 22, 2016 23:21

Fishrock123 merged commit ec6af31 into nodejs:master Mar 22, 2016

joshgav mentioned this pull request Mar 30, 2016

node: Update strings node.js -> bootstrap_node.js #5962

Closed

3 tasks

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 31, 2016

lib,src: move src/node.js to lib/internal/node.js

d168ab5

PR-URL: nodejs#5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Mar 31, 2016

lib: rename /node.js to /bootstrap_node.js

84c28ac

PR-URL: nodejs#5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 mentioned this pull request Mar 31, 2016

Backport refactor src/node.js into internal files #5975

Closed

2 tasks

evanlucas pushed a commit that referenced this pull request Mar 31, 2016

lib,src: refactor src/node.js into internal files

e3c7b46

PR-URL: #5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas pushed a commit that referenced this pull request Mar 31, 2016

lib,src: move src/node.js to lib/internal/node.js

a757e05

PR-URL: #5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas pushed a commit that referenced this pull request Mar 31, 2016

lib: rename /node.js to /bootstrap_node.js

40847b0

PR-URL: #5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas mentioned this pull request Mar 31, 2016

v5.10.0 Release Proposal #5970

Closed

evanlucas pushed a commit that referenced this pull request Mar 31, 2016

lib,src: refactor src/node.js into internal files

32bd75a

PR-URL: #5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas pushed a commit that referenced this pull request Mar 31, 2016

lib,src: move src/node.js to lib/internal/node.js

e571b14

PR-URL: #5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

evanlucas pushed a commit that referenced this pull request Mar 31, 2016

lib: rename /node.js to /bootstrap_node.js

4ecb88f

PR-URL: #5103 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

tomkel mentioned this pull request Apr 6, 2016

No longer compiles as of node >= 5.10.0 nexe/nexe#207

Closed

DavidCai1111 mentioned this pull request Apr 27, 2016

通过源码解析 Node.js 启动时第一个执行的 js 文件：bootstrap_node.js DavidCai1111/my-blog#28

Open

refactor src/node.js into internal files by Fishrock123 · Pull Request #5103 · nodejs/node · GitHub

Uh oh!

Uh oh!

refactor src/node.js into internal files - #5103

refactor src/node.js into internal files#5103Fishrock123 merged 3 commits intonodejs:masternodejs/node:masterfrom Fishrock123:cleanup-node.jsFishrock123/node:cleanup-node.jsCopy head branch name to clipboard

Conversation

Fishrock123 commented Feb 5, 2016

Uh oh!

Fishrock123 Feb 5, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Feb 5, 2016

Uh oh!

vkurchatkin Feb 5, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Feb 5, 2016

Choose a reason for hiding this comment

Uh oh!

vkurchatkin Feb 5, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Feb 6, 2016

Choose a reason for hiding this comment

Uh oh!

chrisdickinson commented Feb 5, 2016

Uh oh!

Fishrock123 commented Feb 6, 2016

Uh oh!

Fishrock123 Feb 6, 2016

Choose a reason for hiding this comment

Uh oh!

rogerwang commented Feb 6, 2016

Uh oh!

Fishrock123 commented Feb 10, 2016

Uh oh!

trevnorris Feb 11, 2016

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Feb 11, 2016

Uh oh!

Fishrock123 commented Feb 11, 2016

Uh oh!

jasnell commented Feb 11, 2016

Uh oh!

Fishrock123 commented Feb 11, 2016

Uh oh!

jasnell commented Feb 11, 2016

Uh oh!

Fishrock123 commented Feb 11, 2016

Uh oh!

jasnell commented Feb 11, 2016

Uh oh!

Fishrock123 commented Feb 11, 2016

Uh oh!

jasnell commented Feb 11, 2016

Uh oh!

jasnell commented Feb 11, 2016

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

Fishrock123 commented Mar 22, 2016

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

refactor src/node.js into internal files#5103
Fishrock123 merged 3 commits into
nodejs:masterfrom
Fishrock123:cleanup-node.js