fix(ext/fetch): Fix missing check of duplex field #17192
Conversation
|
|
|
Hello! Thank you so much for this awesome project! I hope that this is helpful and I hope that I followed all the proper instructions! Please let me know what I can do to clean this up so that is as helpful as possible! Thank you again! |
hawkinsw
force-pushed
the
fetch_init_duplex_fix
branch
from
ec43127
to
7aaa2b0
Compare
|
Forgive my earlier PR with leftover print-debugging statement. Sorry! |
|
Sorry for not leaving the tests in a happy state! I am fixing up the tests now. Sorry again! cc @bartlomieju @lucacasonato |
hawkinsw
force-pushed
the
fetch_init_duplex_fix
branch
from
7aaa2b0
to
1a3eed3
Compare
|
Again, sorry about not fixing the tests the first time that I submitted the PR. The tests should be happy now! (At least they are happy on my machine) Thanks for all the hard work you both do for the project! |
Hope you are all doing well! I just wanted to let you know that I am more than happy to make whatever changes you would like! I hope that you find this helpful! Sincerely, |
There was a problem hiding this comment.
Thanks for the PR @hawkinsw, could you please run format script (tools/format.js) to make CI happy?
Also are there any WPT tests that cover this change?
hawkinsw
force-pushed
the
fetch_init_duplex_fix
branch
from
1a3eed3
to
8ff5649
Compare
I am terribly sorry -- I absolutely thought that I had already done this step! I pushed a new version of the PR. Sorry again!
Not that I am aware of. That said, I will be happy to look and let you know what I find! Thanks again for all your work on the project! |
|
Looks like we fixed some "expected" failures: https://github.com/denoland/deno/actions/runs/3832506757/jobs/6523719259#step:39:16770 I will go through and attempt to remove the "expected failure" marking from those. I may need to reach out if I can't figure out how to wrangle the test configuration. |
WHATWG specifies that when a user gives a `ReadableStream` as the `body` as the `init` parameter of a `Request` constructor, the implementation must check whether the user has also set the `duplex` field to "half".
hawkinsw
force-pushed
the
fetch_init_duplex_fix
branch
from
8ff5649
to
33e0eb1
Compare
|
Hello @bartlomieju !! I have updated I hope that this helps! |
|
We have to diverge from the spec here (this is a breaking change) as I mention in the upstream fetch spec issue. I would like to punt this issue into the future until I have time to specify the behavior of |
@lucacasonato Thanks for the reply! I am not 100% sure what that means with respect to this particular PR. Would you like me to make it so that the possible values for the Also, I was a part of some of the specification work when I was an employee at Mozilla and would love to get involved in it again. Please, please let me know if/how I can help with the fetch spec. After unearthing this minor issue, I do feel like I have a better grasp of the spec! Thanks again for everything -- I am an admirer of you and your work! |
|
Sorry to bother, but I just wanted to check in and see if there was anything that I could do to help! Thanks for everything, @lucacasonato !! |
|
@hawkinsw Can you rebase the branch? Some conflict has unfortunately arisen so I can't trigger tests on this. |
|
It would be my pleasure. I will do it ASAP!
…On Sun, Feb 12, 2023 at 2:29 PM Aapo Alasuutari ***@***.***> wrote:
@hawkinsw <https://github.com/hawkinsw> Can you rebase the branch? Some
conflict has unfortunately arisen so I can't trigger tests on this.
—
Reply to this email directly, view it on GitHub
<#17192 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCP2CSLFAGBAQVJ72A6UULWXE22VANCNFSM6AAAAAATKC5BXY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@hawkinsw Sorry for the very late reply. Let me elaborte: A couple of months ago, Chrome wanted to ship request body streaming in At this point I suggested we introduce the Deno however already defaulted to Additionally, because of backwards compatibility, we can't add the requirement that users must specify So what does that mean for this PR? I see two paths forward:
I am sort of leaning towards option 2. |
|
For the record, it doesn't seem like |
|
Thank you to both of you for the engagement. I am mostly concerned with
being able to use a standard set of code to do feature detection of support
for request body streaming. I am helping to get this used by the Go wasm
backend and we had to write code to detect whether the runtime supports it
(see https://go-review.googlesource.com/c/go/+/458395). We used the
detection code posted by Jake (
https://developer.chrome.com/articles/fetch-streaming-requests/) (after
fixing a bug, which I am sure you immediately see). In order to use that
code we needed to add this patch to Deno.
So, my goals are twofold:
1. To help Deno in any way that I can (!!)
2. Find a reasonable way to implement feature detection for streaming
request bodies that we can incorporate into the Go wasm backend.
I am sorry that is such a circuitous response. You two are the experts. I
am just here to help!
Will
…On Mon, Feb 13, 2023 at 7:15 AM Andreu Botella ***@***.***> wrote:
For the record, it doesn't seem like duplex: "full" will be specified in
the WHATWG fetch spec anytime soon. However, WinterCG
<https://wintercg.org/> is working on a fork of the fetch spec
<https://github.com/wintercg/fetch> meant for server-side runtimes which
will most likely include duplex: "full" at some point.
—
Reply to this email directly, view it on GitHub
<#17192 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCP2CWMVUFGULYSPJ7ERDDWXIQVDANCNFSM6AAAAAATKC5BXY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
WHATWG specifies that when a user gives a
ReadableStreamas thebodyas theinitparameter of aRequestconstructor, the implementation must check whether the user has also set theduplexfield to "half".FYI: Step 39 is where the requirement is defined.