-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Improve error handling #3859
Improve error handling #3859
Conversation
|
69d262a
to
bd7ee31
Compare
E2E tests are failing because of the issue fixed by #3915! |
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.
Some comments, but verror
y impressive!
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 once smoke passes ✅
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.
Left some comments but not blocking / LGTM otherwise
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.
I'm not understanding how __astro_tag_component__
is relevant to error messages. Is it? If its for the sake of speeding up rendering I think that should be done in a separate issue.
We used to cache the result of .check()
in a weakmap. This worked with any framework and didn't rely on AST parsing. I can't remember why we removed that, but if this is being done for performance I'd rather rethink about that approach.
Changes
500
errors (which was really any error that happens on the server)Before
Errors were uncaught, component ran through many renderers, streaming response would hang indefinitely
After
Errors are caught, component only runs through the correct renderer (which errors helpfully), streaming response closes and Vite's error overlay is shown
Testing
Updated tests for error conditions. Unfortunately was unable to automate error overlay tests due to CI limitations.
Docs
QoL improvement