chore: Split Fetch code #2366
chore: Split Fetch code #2366
Conversation
|
@mcollina @KhafraDev Could you please take a look if this is something that makes sense within the undici codebase? Tests are still failing, but I don't want to invest much effort into something that will be rejected anyway. |
| } | ||
| } | ||
|
|
||
| module.exports = { |
There was a problem hiding this comment.
I tried to split this into three separate files initially, but unfortunately that doesn't fly; as currently implemented, there is a circular reference across these three functions, so they can't be split at all. For me that sounds like a code smell, but this is a larger problem than what could be solved in this PR.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2366 +/- ##
==========================================
+ Coverage 85.54% 92.23% +6.68%
==========================================
Files 76 41 -35
Lines 6858 3258 -3600
==========================================
- Hits 5867 3005 -2862
+ Misses 991 253 -738
☔ View full report in Codecov by Sentry. |
|
If you have the fetch spec open, having the entirety of fetch in its own file makes maintaining it much easier. I'd also like to mention that some people have tried going through fetch for "easy" perf gains and there probably aren't that many that would be accepted. |
|
fair point, I'll just close this then. This PR is making me feel miserable anyway :-D |
This relates to Undici code structure
Rationale
I've started looking into https://twitter.com/bengl/status/1715958172400210065 to identify quick perf wins for the hot path, but quickly hit a roadblock, due to "monolithic block of code" style of code, that is hard to penetrate for an outsider. Since inline function declarations in the original code served as a way to pass closure variables, I've reimplemented this part of the code as a class, which exists precisely for carrying this kind of a context.
Changes
This is pretty much the original code, just broken down into more manageable pieces.
Features
N/A
Bug Fixes
N/A
Breaking Changes and Deprecations
Shouldn't be a breaking change
Status