-
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
Combine CLI Errors. #2487
Combine CLI Errors. #2487
Conversation
9a7e219
to
c7ee43d
Compare
Ok, I have taken some artistic license with the errors to try to unify them, as neither TypeScript nor a coloured version of JSError work very well universally. The biggest change is I took a hint from Rust errors which look something like this:
Where TypeScript pretty errors led with the file location of the error, that really breaks up being to read them because on average, we output very long file identifiers that identify local and remote resources. A TypeScript error looks like this (but in colour):
And a JavaScript error looks like this:
Also, I have remapped the top line, start_column, end_column and line contents from JSErrors, so you get authored code instead of transpiled code. |
That follow the TypeScript model, squiggly red for an error, and squiggly teal/cyan for non-errors, where it is indicating a reference of where something is defined. I should be able to finish off the tests today and get it into a green state. |
Ok, I think this is ready for a final review @ry. I didn't tackle the other error kinds display, as these are intrinsically Rust errors and are generally handled differently (usually via a Also, I didn't handle bubbling up the worker JavaScript errors. I suspect it would be more straightforward to do that in a seperate PR now that there is a unified error infrastructure. |
e2bea3f
to
7500a50
Compare
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.
Just a couple nits - looks great!
@ry feedback addressed (and I forgot to test the source line remapping in source map, though it was being covered by the integrations tests). |
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
Resolves #2474
Outside of core::JSError, all other Deno errors are now combined to be just
DenoError
.I need to do further work on this PR, but wanted to get it visible to get any early feedback. Things I still need to do:
cli/errors.rs
has no tests at all, add testsJSError
(JSErrorColor
) to be more like TypeScript diagnosticsAlign the display of the other Deno errors to be more like TypeScript diagnosticsBubble up non-diagnostic errors in the compiler worker