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
fix(ext/fetch): make EventSource
more robust
#22493
fix(ext/fetch): make EventSource
more robust
#22493
Conversation
83d2292
to
f1f6ad8
Compare
f1f6ad8
to
aee159a
Compare
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, though I did leave a few nitpicks. If you want to fix them, by all means do. Otherwise we'll merge this in the coming days.
ext/fetch/27_eventsource.js
Outdated
if (this[_readyState] !== CLOSED) { | ||
this[_readyState] = OPEN; | ||
this.dispatchEvent(new Event("open")); | ||
if (this[_readyState] === CLOSED) { |
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.
praise: Nice inversion of ifs to reduce unnecessary indentation.
ext/fetch/27_eventsource.js
Outdated
if (this[_lastEventID] !== "") { | ||
lastEventIDValue = this[_lastEventID]; | ||
} | ||
this[_reestablishConnection](); |
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.
praise: Excellent deduplication work!
aee159a
to
05bc224
Compare
05bc224
to
aa35d09
Compare
@aapoalas Any ideas why the CI failed? Seems like a flaky test to me. |
Lint failed from clippy not running or finding something fishy. Either main contained something fishy or it's indeed flaky :/ |
8ee5834
to
41a494f
Compare
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 PR fixes all unhandled rejections and resource leaks found while adding a test for #22368.