util: pass on additional error() args#4279
Conversation
There was a problem hiding this comment.
It probably doesn't really matter here but V8 doesn't like it when you modify arguments.
There was a problem hiding this comment.
I thought so too, but according to @petkaantonov's "Optimization killers" page it doesn't apply if the script/function is in strict mode?
There was a problem hiding this comment.
I checked and Crankshaft bails out with 'Bad value context for arguments value' as the deopt reason; TurboFan seems to be able to optimize it, though.
If writing it like this is unavoidable, I'd at least DRY the formatting into const fmt = ${prefix}${msg};
There was a problem hiding this comment.
Would cloning arguments and mutating that instead be better?
There was a problem hiding this comment.
personally I'd be more comfortable with that.
mscdex
force-pushed
the
fix-events-stderr-output
branch
from
ab45899 to
d701af8
Compare
There was a problem hiding this comment.
why not simply Array.prototype.slice.call(arguments) then args[0] = fmt after?
There was a problem hiding this comment.
Because (AFAIK this is still the case) passing arguments to a function other than fn.apply() causes deopts in v8.
There was a problem hiding this comment.
ah.. right, keep forgetting about that. carry on then :-)
There was a problem hiding this comment.
Are we avoiding [...arguments] (or function (...rest) args) to make the LTS backporting process easier?
There was a problem hiding this comment.
Well if for no other reason than I think spread is slow in v8 still? I am not 100% sure if that changed in v8 4.7 or not...
|
LGTM |
This fixes breakage introduced in 94b9948 when writing the max EventEmitter listeners warning to stderr.
mscdex
force-pushed
the
fix-events-stderr-output
branch
from
d701af8 to
4785e7a
Compare
|
CI is all green except unrelated, flaky tests: https://ci.nodejs.org/job/node-test-pull-request/1000/ |
|
Gnarly but LGTM. |
This fixes breakage introduced in 94b9948 when writing the max EventEmitter listeners warning to stderr. PR-URL: #4279 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
|
Landed in 0b43c08. |
|
The referenced commit that is broken 94b9948 is a semver Major change not in the v4.x branch |
|
As this commit has a dont-land-on-v5.x I'm assuming the same is true for v4.x adding the appropriate label |
This fixes breakage introduced in 94b9948 when writing the max EventEmitter listeners warning to stderr. PR-URL: nodejs#4279 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
6 participants
This fixes breakage introduced in 94b9948 when writing the max EventEmitter listeners warning to stderr.