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
refactor: unify close/destroy #1278
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1278 +/- ##
==========================================
- Coverage 94.13% 94.05% -0.08%
==========================================
Files 44 45 +1
Lines 4091 4055 -36
==========================================
- Hits 3851 3814 -37
- Misses 240 241 +1
Continue to review full report at Codecov.
|
} | ||
|
||
if (this[kDestroyed]) { | ||
queueMicrotask(() => callback(new ClientDestroyedError(), null)) |
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 don't think this was an error before in all cases (hence the updates to test). It behaved inconsistently. Not sure what is most correct.
25c3a07
to
33b4fe3
Compare
@mcollina semver-major or not? wdyt? It will now error some close calls that did not error before. |
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 think this should be a semver-major change due to the difference in the error code. Wdyt?
How are the benchmarks before/after?
Benchmarks have only one sample so I don't think they are very accurate but it looks good to me. |
I'm good with semver major |
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 as semver-major
* refactor: unify close/destroy * fixup * fixup * fixup
* refactor: unify close/destroy * fixup * fixup * fixup
* refactor: unify close/destroy * fixup * fixup * fixup
No description provided.