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

Add duplex property to Request #1493

Merged
merged 1 commit into from Sep 26, 2022
Merged

Add duplex property to Request #1493

merged 1 commit into from Sep 26, 2022

Conversation

jakearchibald
Copy link
Collaborator

@jakearchibald jakearchibald commented Sep 23, 2022

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


Preview | Diff

@jakearchibald
Copy link
Collaborator Author

Tests added. I'll create the implementation bugs when this is reviewed.

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. I was initially thinking we might want to de-duplicate some of the domintro with the duplex dictionary member, but for now this actually seems fine as they are somewhat different features even if they share a name. And we can revise when we add "full".

@annevk
Copy link
Member

annevk commented Sep 26, 2022

I think we can land this once there are impl bugs. This is really a bug fix for #1457 as #1486 shows it's rather broken without it.

@annevk
Copy link
Member

annevk commented Sep 26, 2022

cc @lucacasonato

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.

I agree with the motivation for this change.

I do have a concern with this implementation though, namely that this effectively sets the default value for duplex to "half" (even if the user did not specify a value). This is incompatible with Deno, because our default value has always been "full" there (even though this is not explicitly exposed on a .duplex member on the Request object.

I think we'll just have to swallow the "implementation will vary from spec" pill here though, as we implemented full duplex streaming before standardization.

The user observable change here will be:

// Chrome
const req = new Request("Hello World");
req.duplex; // "half"

// Deno
const req = new Request("Hello World");
req.duplex; // "full"

This is not much worse than the status quo though, as the actual behaviour would be different between Deno and other browsers regardless of this PR. This PR just explicitly exposes this difference between the default duplex streaming behaviours in a string value that the user can see.

Also of note: this does not close the door to updating the default for duplex being "full" when a user specifies a ReadableStream body.

LGTM

@jakearchibald
Copy link
Collaborator Author

@annevk

I was initially thinking we might want to de-duplicate some of the domintro with the duplex dictionary member

Yeah, I felt the same, but figured developers may go straight to either bit of documentation, so the duplication seem ok.

@jakearchibald
Copy link
Collaborator Author

@annevk Implementation bugs added.

Could you take a look at web-platform-tests/wpt#36048 so (hopefully) browsers don't break the feature detect?

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 26, 2022
@annevk annevk merged commit 1fbc40c into main Sep 26, 2022
@annevk annevk deleted the add-duplex-property branch September 26, 2022 15:06
annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 26, 2022
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 13, 2022
Automatic update from web-platform-tests
Fetch: Request duplex property

For whatwg/fetch#1493.
--

wpt-commits: 8fdd0c4c592ac976d264f30e87dc26b0150b4f70
wpt-pr: 36051
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 13, 2022
… detect, a=testonly

Automatic update from web-platform-tests
Request upload: test synchronous feature detect

For whatwg/fetch#1493.
--

wpt-commits: 9841e79d6f1301738125a2fbfbc6992c7b8ae366
wpt-pr: 36048
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Oct 19, 2022
Automatic update from web-platform-tests
Fetch: Request duplex property

For whatwg/fetch#1493.
--

wpt-commits: 8fdd0c4c592ac976d264f30e87dc26b0150b4f70
wpt-pr: 36051
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Oct 19, 2022
… detect, a=testonly

Automatic update from web-platform-tests
Request upload: test synchronous feature detect

For whatwg/fetch#1493.
--

wpt-commits: 9841e79d6f1301738125a2fbfbc6992c7b8ae366
wpt-pr: 36048
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.

None yet

3 participants