-
Notifications
You must be signed in to change notification settings - Fork 29.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
stream: make checking pendingcb on WritableStream backward compatible #54142
Conversation
Review requested:
|
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.
Can you add a test?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54142 +/- ##
==========================================
+ Coverage 87.06% 87.07% +0.01%
==========================================
Files 643 643
Lines 181576 181576
Branches 34894 34895 +1
==========================================
+ Hits 158088 158111 +23
+ Misses 16759 16749 -10
+ Partials 6729 6716 -13
|
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
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Landed in fafc845 |
In PR #53438 we tried to fix
stream.finished
to wait calling its callback until all the pending callback has been called. However the patch breaks the users who use a very old version of the stream which does not havependingcb
state on theWritableStream
.I am not sure if we should patch this or we should encourage the user to use a more up to date version.
Fixes: #54131
Refs: #54131 (comment)