-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Streaming: refactor to custom Error classes #28632
Streaming: refactor to custom Error classes #28632
Conversation
This pull request has merge conflicts that must be resolved before it can be merged. |
This enables better handling of the errors rather than just adding properties to the Error instance
68712da
to
b9c4a1b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #28632 +/- ##
========================================
Coverage 85.07% 85.08%
========================================
Files 1038 1061 +23
Lines 28142 28339 +197
Branches 4532 4545 +13
========================================
+ Hits 23943 24112 +169
- Misses 3039 3065 +26
- Partials 1160 1162 +2 ☔ View full report in Codecov by Sentry. |
This pull request has resolved merge conflicts and is ready for review. |
@renchap @ClearlyClaire I think this should be easily mergeable? |
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.
Looks good to me
Co-authored-by: Claire <[email protected]>
Co-authored-by: Renaud Chaput <[email protected]> Co-authored-by: Claire <[email protected]>
Co-authored-by: Renaud Chaput <[email protected]> Co-authored-by: Claire <[email protected]>
This enables better handling of the errors rather than just adding properties to the Error instance. This is currently a draft until #27828 is merged.
We also need to take a deeper look at the
on("error")
handlers for websockets and eventstreams to double check that we're not accidentally sending confidential information about errors to the clients.I will also note that we're approaching the time to do a reorganisation of the
streaming/
directory into astreaming/src/*
andstreaming/index.js
that just starts the server listening, however, due to other inflight work, I don't wish to do this reorganisation yet, as it'd create merge and rebase conflicts.