Skip to content
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

doc:Error:captureStackTrace description inaccurate #5675

Closed
pabigot opened this issue Mar 12, 2016 · 8 comments
Closed

doc:Error:captureStackTrace description inaccurate #5675

pabigot opened this issue Mar 12, 2016 · 8 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. wip Issues and PRs that are still a work in progress.

Comments

@pabigot
Copy link
Contributor

pabigot commented Mar 12, 2016

From at least Node 4.2 the documentation says:

The first line of the trace, instead of being prefixed with ErrorType: message, will be the result of calling targetObject.toString().

This test program:

var assert = require('assert');

function X(msg) {
  this.message = msg;
}
X.prototype.name = 'X';
X.prototype.toString = function() { return 'toString'; }

var x = new X('something');
assert.deepEqual(x, {message: 'something'});
assert.equal(x.toString(), 'toString');

Error.captureStackTrace(x);
assert.equal(x.stack.split('\n')[0], 'X: something');

proves the description incorrect. The first line is produced by deps/v8/src/messages.js in ErrorToStringDetectCycle from the name and message properties of targetObject with this logic:

if (name === "") return message;
if (message === "") return name;
return name + ": " + message;

where name defaults to "Error" and message defaults to "".

I'm not good at user-facing technical documentation so I'd rather not try to craft replacement text for something that complex.

@mscdex mscdex added the doc Issues and PRs related to the documentations. label Mar 12, 2016
@thefourtheye
Copy link
Contributor

I am not sure I understood the actual problem. Can you please explain what the expected result be?

@pabigot
Copy link
Contributor Author

pabigot commented Mar 13, 2016

? The problem is that the behavior of the function is not as documented. In the code I gave targetObject is x. If the documentation was right, the first line of the stack trace would be toString, which is the result of invoking x.toString(). Instead it is X: something, because it's constructed from the name ("X") and message ("something") properties of x.

@thefourtheye
Copy link
Contributor

Okay, this was introduced in 7e2235a. cc @chrisdickinson

@bodenr
Copy link

bodenr commented Mar 31, 2016

I would assert this is a bug (regression), unless the behavior has intentionally changed in more recent versions of node.

For example...

stacks.js

function MyError() {
  Error.captureStackTrace(this, MyError);
}

MyError.prototype.toString = function() {
    return "My error"
}

console.log(new MyError().stack)

Now testing behavior with node 0.10.25 and node 4.4.1:

boden@ubuntu:/tmp/errors$ node -v && node ./stacks.js && node4 -v && node4 ./stacks.js
v0.10.25
My error
    at Object.<anonymous> (/tmp/errors/stacks.js:9:13)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:902:3
v4.4.1
Error
    at Object.<anonymous> (/tmp/errors/stacks.js:9:13)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
    at startup (node.js:139:18)
    at node.js:968:3
boden@ubuntu:/tmp/errors$ 

As shown above, in node 0.10.25 the value of toString() is used for stack (e.g. My error), but in node 4.4.1 it's not and the string Error is used.

There are existing modules which depend on node's captureStackTrace() to use the value from toString() for the stack trace... These modules are now broken with recent versions of node.

@bnoordhuis
Copy link
Member

@bodenr Try MyError.prototype.name = 'My error' or set this.name = 'My error' in the constructor.

As to Error.captureStackTrace(), it's implemented by V8, node only documents it. The way it formats exception messages now looks ES6-compliant to me (v0.10 probably isn't) so presumably it only needs a documentation update on our side.

@Fishrock123
Copy link
Contributor

See also e2f47f5698?

But, what @bnoordhuis said, you need have .name set.

@Trott
Copy link
Member

Trott commented Jul 7, 2017

Anyone from @nodejs/documentation want to submit the doc fix to close this issue? Alternatively, put a good first contribution label on it?

@refack
Copy link
Contributor

refack commented Jul 7, 2017

So currently the docs are wrong

224 | The first line of the trace, instead of being prefixed with `ErrorType:
225 | message`, will be the result of calling `targetObject.toString()`.

Should probably be

224 | The first line of the trace will be prefixed with `ErrorType.name: message`.

@refack refack added the good first issue Issues that are suitable for first-time contributors. label Jul 7, 2017
romanshoryn added a commit to romanshoryn/node that referenced this issue Jul 9, 2017
@refack refack added the wip Issues and PRs that are still a work in progress. label Jul 10, 2017
romanshoryn added a commit to romanshoryn/node that referenced this issue Jul 11, 2017
@refack refack closed this as completed in 7f26a29 Jul 12, 2017
addaleax pushed a commit that referenced this issue Jul 18, 2017
PR-URL: #14150
Fixes: #5675
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Kunal Pathak <[email protected]>
Fishrock123 pushed a commit that referenced this issue Jul 19, 2017
PR-URL: #14150
Fixes: #5675
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Kunal Pathak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants