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(fetch): spec #1295
fix(fetch): spec #1295
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@KhafraDev interested in helping out with this one? |
test/fetch/data-uri.js
Outdated
@@ -1,215 +1,215 @@ | |||
'use strict' |
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.
@KhafraDev help here? these fail...
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.
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
let body = stringPercentDecode(encodedBody) |
Line 67 in 6a0388f
body = forgivingBase64(stringBody) |
So then, possibly either a spec oversight or my mistake, the body is never extracted:
Line 879 in 6a0388f
body: dataURLStruct.body |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@KhafraDev thanks! Seems like a spec bug that it's missing extract body here.
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.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
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
Applies some recent spec refactoring. Doesn't fix anything per se.