Give more context to unhandled error events #1654
Conversation
|
👍 I like this so lgtm |
|
This almost encourages people to use strings as errors. I like reporting the data that was provided. What if you added it to end of the default error's message? |
| } else { | ||
| // At least give some kind of context to the user | ||
| console.error(er); |
There was a problem hiding this comment.
I'm not so sure about this. Can't we just attach er to the thrown error object and/or concatenate the er to the thrown error message?
There was a problem hiding this comment.
so instead do something like so?:
er += (new Error()).stack.substr(6);There was a problem hiding this comment.
Wouldn't that make er a string though?
There was a problem hiding this comment.
Got this change and the one above it mixed up in context. nm what I said.
|
@cjihrig That could work. I understand the concern about encouraging throwing strings. Sometimes, strings are thrown from dependencies. |
|
With regards to string handling, I agree with @cjihrig. IMHO it's better to just submit PRs to those dependencies that are throwing strings. |
|
Ok, so maybe set whatever |
|
@evanlucas Yeah something like, I'm not sure what the best terminology would be...
|
|
Ok, so we add a property to the error and if the argument is a string, append it to the error message? And we want to not actually log the argument? |
mscdex
added
the
events
|
We should probably append the argument, no matter what the type. Seeing the bad value (with strings being the most likely case) might help with debugging. |
|
Ok, updated to append the argument to the default error message. The argument is also added to the error object as the |
|
LGTM if everyone is OK with using |
|
Thanks @cjihrig. I'll leave open for a few days to make sure everyone has a chance to voice his or her opinion. I will plan on merging on Tuesday if there are no objections. |
Previously, in the event of an unhandled error event, if the error is a not an actual Error, then a default error is thrown. Now, the argument is appended to the error message and added as the `context` property of the error. PR-URL: nodejs#1654 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
Landed in 8b9a153 |
PR-URL: nodejs#1679 Notable Changes: * win,node-gyp: the delay-load hook for windows addons has now been correctly enabled by default, it had wrongly defaulted to off in the release version of 2.0.0 (Bert Belder) nodejs#1433 * os: tmpdir()'s trailing slash stripping has been refined to fix an issue when the temp directory is at '/'. Also considers which slash is used by the operating system. (cjihrig) nodejs#1673 * tls: default ciphers have been updated to use gcm and aes128 (Mike MacCana) nodejs#1660 * build: v8 snapshots have been re-enabled by default as suggested by the v8 team, since prior security issues have been resolved. This should give some perf improvements to both startup and vm context creation. (Trevor Norris) nodejs#1663 * src: fixed preload modules not working when other flags were used before --require (Yosuke Furukawa) nodejs#1694 * dgram: fixed send()'s callback not being asynchronous (Yosuke Furukawa) nodejs#1313 * readline: emitKeys now keeps buffering data until it has enough to parse. This fixes an issue with parsing split escapes. (Alex Kocharin) * cluster: works now properly emit 'disconnect' to cluser.worker (Oleg Elifantiev) nodejs#1386 events: uncaught errors now provide some context (Evan Lucas) nodejs#1654
Previously, in the event of an unhandled error event, if the error is a not an actual Error, then a default error is thrown. Now, the argument is appended to the error message and added as the `context` property of the error. PR-URL: nodejs#1654 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Previously, in the event of an unhandled error event, if the error is a
not an actual Error, then a default error is thrown. Now, if a string
is the unhandled error, it is wrapped in an Error and then thrown.
This provides more context in the event a string is emitted for the
error event.
This PR also prints the error to stderr in the event it is not a string or an Error object.
I see this as being controversial, but it was quite minimal so I wanted to see what everyone else thinks.
/cc @iojs/collaborators