-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
fix(ext/console): Error Cause Not Inspect-Formatted when printed #24526
fix(ext/console): Error Cause Not Inspect-Formatted when printed #24526
Conversation
Signed-off-by: MujahedSafaa <168719085+MujahedSafaa@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR seems to be missing a test case that exercises the change and shows why it was made. What happens when the .stack
property holds something that JSON.stringify
cannot deal with like a bigint?
The code run JSONstringify only in case the stack value is null or undefined. |
Try running this snippet with the changes in this PR. On my end it causes an unexpected exception: console.log(new Error("foo", { cause: 9007199254740991n })); Output: error: Uncaught (in promise) TypeError: Do not know how to serialize a BigInt
console.log(new Error("foo", { cause: 9007199254740991n }));
^
at stringify (<anonymous>)
at ext:deno_console/01_console.js:1496:13
at Array.map (<anonymous>)
at inspectError (ext:deno_console/01_console.js:1490:5)
at formatRaw (ext:deno_console/01_console.js:852:16)
at formatValue (ext:deno_console/01_console.js:530:10)
at inspectArgs (ext:deno_console/01_console.js:3044:17)
at console.log (ext:deno_console/01_console.js:3113:7)
at file:///Users/marvinh/dev/test/deno-cause/main.ts:1:9 Deno should not error like that. To make it work we must ensure that any JS value can be printed, not just a subset. |
Yes, it causes an error also on my side, I will add a fix to it. |
@marvinhagemeister |
StringPrototypeReplace( | ||
inspect(cause), | ||
doubleQuoteRegExp, | ||
"", | ||
)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inspect
does not insert superfluous double quotes like JSON.stringify
does, so the replace
call can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
) This pull request addresses an issue where the Error.cause property was not formatted correctly when printed using console.log, leading to confusion. solution: Implemented a fix to ensure that Error.cause is formatted properly when printed by console.log, and the fix done by using JSON.stringify This PR fixes #23416 --------- Signed-off-by: MujahedSafaa <168719085+MujahedSafaa@users.noreply.github.com>
This pull request addresses an issue where the Error.cause property was not formatted correctly when printed using console.log, leading to confusion.
solution:
Implemented a fix to ensure that Error.cause is formatted properly when printed by console.log, and the fix done by using JSON.stringify
This PR fixes #23416