Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1295 +/- ##
==========================================
+ Coverage 94.03% 94.16% +0.12%
==========================================
Files 44 44
Lines 4107 4145 +38
==========================================
+ Hits 3862 3903 +41
+ Misses 245 242 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@KhafraDev interested in helping out with this one? |
| @@ -1,215 +1,215 @@ | |||
| 'use strict' | |||
There was a problem hiding this comment.
Okay this is quite an easy fix, but difficult to track down (my favorite)!
dataURLProcessor returns either a string or a Uint8Array
Line 57 in 6a0388f
Line 67 in 6a0388f
So then, possibly either a spec oversight or my mistake, the body is never extracted:
Line 879 in 6a0388f
Which means it's now a Uint8Array (most likely). The fix is just adding extractBody(dataURLStruct.body)[0]. All tests pass as well.
There was a problem hiding this comment.
@KhafraDev thanks! Seems like a spec bug that it's missing extract body here.
There was a problem hiding this comment.
This one is on us I'm pretty sure:
A body consists of:
* A stream (a [ReadableStream](https://streams.spec.whatwg.org/#readablestream) object).
* A source (null, a [byte sequence](https://infra.spec.whatwg.org/#byte-sequence), a [Blob](https://w3c.github.io/FileAPI/#dfn-Blob) object, or a [FormData](https://xhr.spec.whatwg.org/#formdata) object), initially null.
* A length (null or an integer), initially null.
undici only works if the body is a stream, but it should work for all of the other types as well. dataURLProcessor returns a byte sequence body (Uint8Array in our case).
Maybe extractBody handles this?
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
We could just freeze it but I suspect in the future we will need to be able to mutate the headers list for e.g. set-cookie.
|
Hey @ronag. I've noticed this PR reduced the This is also proven by CI: https://github.com/nodejs/undici/actions/runs/2015610582 |
|
I would assume so, From the results it looks like more than 10%, unless I'm reading it wrong? Also a benchmark not so long ago had it at 1k request a second 😐). There is a lot of low hanging fruit I can take a stab at removing (CORs, TAO, Window/ServiceWorker stuff, etc)... |
|
I’m currently prioritizing correctness and maintenance over perf. For perf I recommend the much faster undici api’s. That being said. I’m happy to accept pr’s that improve perf as long as it doesn’t conflict with the prior priorities. Replacing proxy might be a good low hanging fruit. |
* fix(fetch): spec * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fiuxp * fixup * fixup * fiuxp * fiuxp * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup
* fix(fetch): spec * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fiuxp * fixup * fixup * fiuxp * fiuxp * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup
* fix(fetch): spec * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fiuxp * fixup * fixup * fiuxp * fiuxp * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup * fixup
5 participants
Applies some recent spec refactoring. Doesn't fix anything per se.