feat(astro): Add tracking of errors during HTML streaming #15995
Conversation
| } | ||
| } catch (e) { | ||
| sendErrorToSentry(e); | ||
| controller.error(e); |
There was a problem hiding this comment.
I think we should still rethrow this error, otherwise we risk breaking people who rely on this.
Is controller.error(e); robust enough?
There was a problem hiding this comment.
controller.error is what rethrows the error on a stream. Throwing from this function is ignored (it was being ignored before, so no one was relying on the error working).
The test is checking that the error is being forwarded.
There was a problem hiding this comment.
Hmm I'm not sure if we want to capture this error at all. The only reason we're even going through the body is to inject our <meta> tags and if it's us causing the error by doing so, we shouldn't report it.
Or would we even swallow errors if we didn't modify the HTML?
The try/catch is probably a good idea anyway to degrade gracefully 🤔
There was a problem hiding this comment.
Hmm I'm not sure if we want to capture this error at all.
This is a user thrown error that is being missed and shows as Internal Server Error on rendering. I think it makes sense for Sentry to capture and report it, but most of all it makes sense to not drop it and pass it forward so errors from the user are not discarded.
We could have some more verbose code to handle only errors from the iterator and not from any part of the <meta> tag inside the loop.
There was a problem hiding this comment.
Discussed this with @AbhiPrasad offline and for now we think it's fine to leave as-is. The injection is fairly low risk (famous last words lol). In case we get reports about our HTML injection logic causing errors, we can still revisit.
Thanks for contributing!
There was a problem hiding this comment.
Thank you @Fryuni! We'll try and get this out with the next release
This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #15995 Co-authored-by: AbhiPrasad <18689448+AbhiPrasad@users.noreply.github.com>
Astro components are rendered in a stream, so if a component that is not the page fails during a request, this error is not thrown by the
await next()call, only when the component is reached in the response stream.The code was iterating over the response stream assuming it was infallible, so component errors were being ignored by sentry and hidden from the user. This PR wraps the iteration on a
try/catchpair to report the error and forward it along to the error handling pipeline.Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).