Skip to content
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

Introduce RequestInit.duplex #1457

Merged
merged 7 commits into from
Jul 1, 2022
Merged

Introduce RequestInit.duplex #1457

merged 7 commits into from
Jul 1, 2022

Conversation

yutakahirano
Copy link
Member

@yutakahirano yutakahirano commented Jun 20, 2022

Introduce the member, and throw if init.duplex is not set in the
Request constructor, as discussed at #1438.

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

Introduce the member, and throw if init.duplex is not set in the
Request constructor, as discussed at #1438.
@yutakahirano yutakahirano mentioned this pull request Jun 20, 2022
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add a Request#duplex attribute too, otherwise the new Response(resp.body, resp); pattern breaks.

fetch.bs Outdated Show resolved Hide resolved
Co-authored-by: Luca Casonato <hello@lcas.dev>
@yutakahirano
Copy link
Member Author

yutakahirano commented Jun 20, 2022

We need to add a Request#duplex attribute too, otherwise the new Response(resp.body, resp); pattern breaks.

I don't understand, this change is solely for requests, not responses.

@lucacasonato
Copy link
Member

Sorry, I meant new Request(req.url, req);

@lucacasonato
Copy link
Member

Thinking about that more, the pattern is currently unusable in runtimes that don't support stream request bodies. That means this would be a Deno specific issue (which will be addressed by us defaulting RequestInit#duplex to "full" for requests with body: ReadableStream for now).

Exposing Request#duplex would make this pattern work on the client too, like it already does for responses, so I think we should add it regardless.

@yutakahirano
Copy link
Member Author

As you see in the change, currently duplex = 'full' is unconditionally rejected in the constructor. I expect you to fix that in #1254. I expect you to define Request.duplex altogether.

Does this make sense?

@lucacasonato
Copy link
Member

I expect you to fix that in #1254.

Yeah, that is my understanding too.

I expect you to define Request.duplex altogether.

I don't quite understand. Are you saying to not introduce the Request.duplex attribute in this PR, and instead do that as part of #1254? I'd rather see Request.duplex added at the same time that RequestInit.duplex is added (so in this PR).

@yutakahirano
Copy link
Member Author

I expect you to define Request.duplex altogether.

I don't quite understand. Are you saying to not introduce the Request.duplex attribute in this PR, and instead do that as part of #1254?

Right, because I don't see a reason to specify it now, and I see a reason not to specify it now.

Specifically,

  1. Until Fetch body streams are not full duplex #1254 is resolved, web developers cannot use Request.duplex effectively, as it always returns 'half'.
  2. If we fail to specify Fetch body streams are not full duplex #1254, it will be difficult to remove the member without compatibility problems.

This is different from the situation for RequestInit.duplex.

  1. RequestInit.duplex forces web developers to specify the member (when the body is a stream), and that will allow us not to specify the default value for now.
  2. We can remove RequestInit.duplex if we fail to specify Fetch body streams are not full duplex #1254. There will not be any compatibility problems.

@yutakahirano
Copy link
Member Author

Sorry, I meant new Request(req.url, req);

You can use new Request(req);

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that makes sense to me.

LGTM

yutakahirano added a commit to web-platform-tests/wpt that referenced this pull request Jun 20, 2022
@yutakahirano
Copy link
Member Author

Thank you! @annevk , can you take a look at this change?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good to me, but we can simplify it a bit further.

I like that we have a note about specifying "full" though. Let's keep that.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Jun 28, 2022

I'll merge this on Friday unless anything new comes up. (I assume we'll have the tests sorted by then and @lucacasonato will have had a chance to look at those as well.)

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 1, 2022
@annevk annevk merged commit edf07e5 into main Jul 1, 2022
@annevk annevk deleted the yhirano/duplex branch July 1, 2022 08:13
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 4, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 4, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 4, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 4, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 4, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 5, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 5, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 5, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 5, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 5, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 5, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
aarongable pushed a commit to chromium/chromium that referenced this pull request Jul 5, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740889
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1020764}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 5, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740889
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1020764}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jul 5, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740889
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1020764}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 5, 2022
…only

Automatic update from web-platform-tests
Fetch: tests for RequestInit's duplex

See whatwg/fetch#1457 for context.
--

wpt-commits: a9b73f530b73d237b0512968715104c96631c22a
wpt-pr: 34496
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Jul 7, 2022
…only

Automatic update from web-platform-tests
Fetch: tests for RequestInit's duplex

See whatwg/fetch#1457 for context.
--

wpt-commits: a9b73f530b73d237b0512968715104c96631c22a
wpt-pr: 34496
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jul 8, 2022
Automatic update from web-platform-tests
Introduce RequestInit.duplex

This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740889
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1020764}

--

wpt-commits: 341bf2487ba47b9810a4dfa97752ffe57fa6894f
wpt-pr: 34686
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Jul 11, 2022
Automatic update from web-platform-tests
Introduce RequestInit.duplex

This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740889
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1020764}

--

wpt-commits: 341bf2487ba47b9810a4dfa97752ffe57fa6894f
wpt-pr: 34686
@annevk annevk mentioned this pull request Sep 26, 2022
3 tasks
annevk pushed a commit that referenced this pull request Sep 26, 2022
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
This implements whatwg/fetch#1457.

Bug: 1337696
Change-Id: I3fcf6f484dc922f5a875ed658adad33631d55115
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3740889
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1020764}
NOKEYCHECK=True
GitOrigin-RevId: b4fc85b816b6e74d4c9ef60572bd66026c44d26a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants