-
-
Notifications
You must be signed in to change notification settings - Fork 4k
chore(WebSocketShard): improve zlib-sync errors #10808
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
I can't reproduce that test failure locally, not sure what happened there? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10808 +/- ##
==========================================
- Coverage 39.66% 37.97% -1.69%
==========================================
Files 249 251 +2
Lines 15206 15987 +781
Branches 1456 1477 +21
==========================================
+ Hits 6031 6071 +40
- Misses 9163 9904 +741
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
new Error(`${this.zLibSyncInflate.err}${this.zLibSyncInflate.msg ? `: ${this.zLibSyncInflate.msg}` : ''}`), | ||
); | ||
// Must be here because zlib-sync is lazily loaded | ||
const ZlibErrorCodes = { |
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.
Cache this in a variable outside the class please, so its computed only once
Please describe the changes this PR makes and why it should be merged:
The current error thrown from
zlib-sync
isn't great, and doesn't match what errors fromnode:zlib
look like. This brings them more in line withnode:zlib
errors.Status and versioning classification: