GeezerVison

(fixed linting)

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 ^^^

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

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" [email protected]
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!

landed, thanks everyone!

@Fishrock123 ... woot!