util: pass on additional error() args #4279
Conversation
| @@ -18,7 +18,11 @@ exports.printDeprecationMessage = function(msg, warned) { | |||
| }; | |||
|
|
|||
| exports.error = function(msg) { | |||
| console.error(`${prefix}${msg}`); | |||
| if (arguments.length > 1) { | |||
| arguments[0] = `${prefix}${msg}`; | |||
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
| const args = new Array(arguments.length); | ||
| args[0] = fmt; | ||
| for (let i = 1; i < arguments.length; ++i) | ||
| args[i] = arguments[i]; |
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]>
This fixes breakage introduced in 94b9948 when writing the max EventEmitter listeners warning to stderr.