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

refactor src/node.js into internal files #5103

Merged
merged 3 commits into from Mar 22, 2016

Conversation

Projects
None yet
@Fishrock123
Copy link
Member

Fishrock123 commented Feb 5, 2016

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

@Fishrock123

View changes

lib/internal/process/promises.js Outdated
scheduleMicrotasks = sMt;
NativeModule = NM;
return exports
}

This comment has been minimized.

@Fishrock123

Fishrock123 Feb 5, 2016

Author Member

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

@Fishrock123 Fishrock123 force-pushed the Fishrock123:cleanup-node.js branch Feb 5, 2016

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Feb 5, 2016

(fixed linting)

@vkurchatkin

View changes

lib/internal/process/nextTick.js Outdated

exports.setup = setupNextTick;

function setupNextTick(NativeModule) {

This comment has been minimized.

@vkurchatkin

vkurchatkin Feb 5, 2016

Member

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

This comment has been minimized.

@Fishrock123

Fishrock123 Feb 5, 2016

Author Member

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

This comment has been minimized.

@vkurchatkin

vkurchatkin Feb 5, 2016

Member

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

This comment has been minimized.

@Fishrock123

Fishrock123 Feb 6, 2016

Author Member

Ooooh good catch

@chrisdickinson

This comment has been minimized.

Copy link
Contributor

chrisdickinson commented Feb 5, 2016

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.

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Feb 6, 2016

cc @zcbenz / @rogerwang ^^^

@Fishrock123

View changes

lib/internal/process/promises.js Outdated
scheduleMicrotasks = sMt;
NativeModule = NM;
return exports;
};

This comment has been minimized.

@Fishrock123

Fishrock123 Feb 6, 2016

Author Member

(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?

@Fishrock123 Fishrock123 force-pushed the Fishrock123:cleanup-node.js branch 2 times, most recently Feb 6, 2016

@rogerwang

This comment has been minimized.

Copy link
Member

rogerwang commented Feb 6, 2016

Thanks for CCing @chrisdickinson @Fishrock123 .

It's fine for NW.js.

@Fishrock123 Fishrock123 force-pushed the Fishrock123:cleanup-node.js branch Feb 10, 2016

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Feb 10, 2016

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

@trevnorris

View changes

lib/internal/process/nextTick.js Outdated
function setupNextTick() {
const promises = require('internal/process/promises');
const emitPendingUnhandledRejections = promises.setup(scheduleMicrotasks);
var nextTickQueue = [];

This comment has been minimized.

@trevnorris

trevnorris Feb 11, 2016

Contributor

could be made const.

EDIT: nope. sorry.

@trevnorris

View changes

lib/internal/process/nextTick.js Outdated

// *Must* match Environment::TickInfo::Fields in src/env.h.
var kIndex = 0;
var kLength = 1;

This comment has been minimized.

@trevnorris

trevnorris Feb 11, 2016

Contributor

though these could

This comment has been minimized.

@Fishrock123

Fishrock123 Feb 11, 2016

Author Member

ooooh true I should go through and do these changes since this is a diff anyways

@trevnorris

This comment has been minimized.

Copy link
Contributor

trevnorris commented Feb 11, 2016

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

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Feb 11, 2016

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

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Feb 11, 2016

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).

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Feb 11, 2016

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

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Feb 11, 2016

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 :-)

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Feb 11, 2016

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.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Feb 11, 2016

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/

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Feb 11, 2016

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.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Feb 11, 2016

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.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Feb 11, 2016

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

Fishrock123 added some commits Mar 15, 2016

lib,src: refactor src/node.js into internal files
PR-URL: #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
PR-URL: #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
PR-URL: #5103
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@Fishrock123 Fishrock123 force-pushed the Fishrock123:cleanup-node.js branch to ec6af31 Mar 22, 2016

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

@Fishrock123

This comment has been minimized.

Copy link
Member Author

Fishrock123 commented Mar 22, 2016

landed, thanks everyone!

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Mar 23, 2016

@Fishrock123 ... woot!

@joshgav joshgav referenced this pull request Mar 30, 2016

Closed

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

2 of 3 tasks complete

joshgav added a commit to joshgav/node that referenced this pull request Mar 30, 2016

node: fix comments and strings
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`

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

lib,src: refactor src/node.js into internal files
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

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

@Fishrock123 Fishrock123 referenced this pull request Mar 31, 2016

Closed

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

2 of 2 tasks complete

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

lib,src: refactor src/node.js into internal files
PR-URL: #5103
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

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

lib,src: move src/node.js to lib/internal/node.js
PR-URL: #5103
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

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

lib: rename /node.js to /bootstrap_node.js
PR-URL: #5103
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

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

lib,src: refactor src/node.js into internal files
PR-URL: #5103
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

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

lib,src: move src/node.js to lib/internal/node.js
PR-URL: #5103
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

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

lib: rename /node.js to /bootstrap_node.js
PR-URL: #5103
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@AlphaWong

This comment has been minimized.

Copy link

AlphaWong commented on lib/internal/process/stdio.js in 015cef2 Apr 13, 2017

Why do not use err instead of er ?

This comment has been minimized.

Copy link
Member

sam-github replied Apr 13, 2017

both are common in node core, though err beats er by 350 to 43 by my quick count

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment