-
Notifications
You must be signed in to change notification settings - Fork 565
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
chore: less async await #2464
chore: less async await #2464
Conversation
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
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2464 +/- ##
==========================================
+ Coverage 85.54% 85.62% +0.07%
==========================================
Files 76 77 +1
Lines 6858 7053 +195
==========================================
+ Hits 5867 6039 +172
- Misses 991 1014 +23 ☔ View full report in Codecov by Sentry. |
@@ -82,40 +82,38 @@ class ProxyAgent extends DispatcherBase { | |||
this[kClient] = clientFactory(resolvedUrl, { connect }) | |||
this[kAgent] = new Agent({ | |||
...opts, | |||
connect: async (opts, callback) => { | |||
connect: (opts, callback) => { |
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 is unnecessary change
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.
tbh. This is for discussing. I agree this is reducing the maintainability. I am unsure if we save one Promise by this.
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.
Except for the connect case I don't mind the changes per se. I just don't think this will make any difference whatsoever in real world application. Maybe the consume change might have some value in terms of performance.
In general I'm starting to see a lot of PR's now that are trying to optimize cold paths, which IMHO is a waste of everyones time. |
Co-authored-by: Toni Villena <tonivj5@Gmail.com>
cancel (reason) { | ||
return iterator.return() |
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.
This might hide it from the stacktrace, not sure the attempt is to keep that function hidden as is part of the RedeableStream
API and can be helpful while debugging.
@@ -1807,11 +1807,11 @@ async function httpNetworkFetch ( | |||
async start (controller) { | |||
fetchParams.controller.controller = controller | |||
}, | |||
async pull (controller) { | |||
await pullAlgorithm(controller) | |||
pull (controller) { |
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.
cc: @KhafraDev
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.
the streams spec will wrap this in a promise, so it does nothing
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've changed my mind, I just had to track down an error that was caused by similar changes. For example, if something synchronously throws an error, it can now get swallowed - this is what happened in #2482. It's super annoying to debug and makes more work for us.
Reopening #2463 for a better discussion ;).
This relates to...
Rationale
Changes
Features
Bug Fixes
Breaking Changes and Deprecations
Status