util: Change how Error objects are formatted by zeusdeux · Pull Request #4582 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation20 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Previously, Error objects were formatted as the result of a toString
call bounded by square brackets. They are now formatted as the stack
trace for the given error object. The intention initially was to emulate
how browsers do console.error but since that would also impact
console.warn, console.log, etc, it was decided to make the change at
util.inspect level which is in turn used by the console package.

This is definitely a semver-major change.

Bigger discussion @ #4452

targos added util

Issues and PRs related to the built-in util module.

semver-major

PRs that contain breaking changes and should be released in the next major version.

labels

Jan 8, 2016

jasnell

@@ -55,13 +55,16 @@ assert.equal(util.format('%%%s%%%%', 'hi'), '%hi%%');
})();

// Errors
assert.equal(util.format(new Error('foo')), '[Error: foo]');
const err = new Error('foo');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're working in this file, can you change the require() statements at the top to use const as well?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this in both test-util-format.js and test-util-inspect.js.

LGTM
But I'd like to get other @nodejs/ctc opinions also

LGTM-ish in that I think it's a useful change. Less LGTM in that it probably doesn't format well when you util.inspect() an object graph with an Error object a few levels deep.

@bnoordhuis It does format kinda weird when there is an error object as a part of another object/array but the behaviour is now similar to that in browsers. It looks something like below:

{ a: 10, b: { c: 'nope', d: Error at repl:1:40 at REPLServer.defaultEval (repl.js:252:27) at bound (domain.js:287:14) at REPLServer.runBound [as eval] (domain.js:300:12) at REPLServer. (repl.js:417:12) at emitOne (events.js:83:20) at REPLServer.emit (events.js:170:7) at REPLServer.Interface._onLine (readline.js:215:10) at REPLServer.Interface._line (readline.js:554:8) at REPLServer.Interface._ttyWrite (readline.js:831:14) } }

Previously, Error objects were formatted as the result of a toString call bounded by square brackets. They are now formatted as the stack trace for the given error object. The intention initially was to emulate how browsers do console.error but since that would also impact console.warn, console.log, etc, it was decided to make the change at util.inspect level which is in turn used by the console package.

Fixes: nodejs#4452

zeusdeux

@@ -488,7 +488,7 @@ function formatPrimitiveNoColor(ctx, value) {


function formatError(value) {
return '[' + Error.prototype.toString.call(value) + ']';
return value.stack \|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added fallback in case the error is some custom error that isn't capturing the stack correctly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a regression test for that? EDIT: Nevermind, I see BadCustomError covers that.

LGTM. One more sign-off would be good.

1 similar comment

jasnell pushed a commit that referenced this pull request

Jan 11, 2016

Previously, Error objects were formatted as the result of a toString call bounded by square brackets. They are now formatted as the stack trace for the given error object. The intention initially was to emulate how browsers do console.error but since that would also impact console.warn, console.log, etc, it was decided to make the change at util.inspect level which is in turn used by the console package.

Fixes: #4452 PR-URL: #4582 Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Reviewed-By: Trevor Norris trev.norris@gmail.com Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: James M Snell jasnell@gmail.com

scovetta pushed a commit to scovetta/node that referenced this pull request

Apr 2, 2016

Previously, Error objects were formatted as the result of a toString call bounded by square brackets. They are now formatted as the stack trace for the given error object. The intention initially was to emulate how browsers do console.error but since that would also impact console.warn, console.log, etc, it was decided to make the change at util.inspect level which is in turn used by the console package.

Fixes: nodejs#4452 PR-URL: nodejs#4582 Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Reviewed-By: Trevor Norris trev.norris@gmail.com Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: James M Snell jasnell@gmail.com

jasnell added a commit that referenced this pull request

Apr 26, 2016