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

refactor(cli/fmt_errors): Format stack frames in prepareStackTrace() #4706

Merged
merged 3 commits into from
Apr 11, 2020

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Apr 10, 2020

Ref #4690 (comment), cc @ry @piscisaureus.

This implements the coloring in prepareStackTrace() and passes the formatted strings to Rust via error.__formattedFrames.

Since the formatting in prepareStackTrace() is properly aligned with V8, this fixes 1/3 and 2/3 of #4705. 3/3 (which involves a built-in function frame) requires making line and column numbers optional in Rust and then just removing the null guard, I'll see about that later.

The source line formatting is still duplicated in core/js_error.rs and cli/fmt_errors.rs.

@kitsonk
Copy link
Contributor

kitsonk commented Apr 11, 2020

I know it isn't directly related to this, but we should start using std/fmt/colors.ts instead of the js/colors.ts and get rid of it. It is a hold over from when it was difficult/impossible to use std in internal Deno code.

@ry
Copy link
Member

ry commented Apr 11, 2020

We still don't have any imports from core into std. Hopefully it would work easily - but I wouldn't be surprised if there were unexpected difficulties.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Sometimes we have exception objects in Rust and want to print them. Doesn't this make it so we can only print exceptions from JS?

Also seems to remove tests...

@nayeemrmn
Copy link
Collaborator Author

nayeemrmn commented Apr 11, 2020

Sometimes we have exception objects in Rust and want to print them. Doesn't this make it so we can only print exceptions from JS?

How so? The whole point is to pass the formatted string from JS to Rust. The impl fmt::Display for JSError is still there and has the same kind of output.

Also seems to remove tests...

Yeah, some functionality was moved from those modules to error_stack.ts. error_stack_test.ts already has tests for source mapping, I need to add another to replace the two to_string tests I removed and that should suffice. Although I guess that's well covered in the integration tests... but the #4705 cases need tests.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 2b362be into denoland:master Apr 11, 2020
@nayeemrmn nayeemrmn deleted the error-format-dedup branch April 12, 2020 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants