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

Feature detecting request stream bodies #1275

Closed
jakearchibald opened this issue Jul 29, 2021 · 9 comments
Closed

Feature detecting request stream bodies #1275

jakearchibald opened this issue Jul 29, 2021 · 9 comments

Comments

@jakearchibald
Copy link
Collaborator

I'm trying to figure out a way to feature-detect request streaming without requiring a network connection.

Unfortunately Safari complicates this, as it supports creating Request objects with stream bodies, but it doesn't support using them with fetch().

However, I've come up with the following:

const supportsRequestStreams = (async () => {
  const supportsStreamsInRequestObjects = !new Request('', {
    body: new ReadableStream(),
    method: 'POST',
  }).headers.has('Content-Type');

  if (!supportsStreamsInRequestObjects) return false;

  return fetch('data:a/a;charset=utf-8,', {
    method: 'POST',
    body: new ReadableStream(),
  }).then(() => true, () => false);
})();

Browsers that don't support streams in request objects would interpret the ReadableStream as a string, and therefore set the Content-Type to text/plain;charset=UTF-8, so that's an early exit.

Otherwise, it performs a fetch to a data url using a stream body. This depends on a few things:

  • POSTing to a data-url is fine, even if the body is a stream. The spec support this, and browsers seem to allow it.
  • Browsers that support streams in request objects, but don't support fetching them, will reject even if the body isn't used (it isn't used when POSTing to a data url). There's nothing in the spec to handle this currently.
  • Browsers don't try to consume the stream (it never closes). Although, I guess I could just close it.

Proposal:

  • Add a step to https://fetch.spec.whatwg.org/#fetch-method to cater for the current Safari behaviour where it rejects requests with streaming bodies. Maybe add a note to suggest that UAs shouldn't create this mismatch in future?
  • Add wpts to cover the feature detect above.

WDYT?

@annevk
Copy link
Member

annevk commented Jul 29, 2021

Why should the specification allow for Safari's behavior?

@jakearchibald
Copy link
Collaborator Author

I'm not set on adding that bit to the spec, I just want to ensure the feature detect is more likely to work long term.

Right now they should move their point of rejection so it happens later than Scheme Fetch, which means the feature detect reports a false positive. I want to avoid that as much as possible.

@annevk
Copy link
Member

annevk commented Jul 29, 2021

Couldn't they adopt the stringification behavior still? At least until they get around to supporting upload streams.

In any event, I'm not aware of specifications defining what should happen for half support of features. You either have to implement it or not support it at all.

@jakearchibald
Copy link
Collaborator Author

As in, ask them to remove support for streams in Request objects until they're ready to support it in fetch? Happy to do that, but doesn't that leave us crossing-fingers that the feature detect in the OP will continue to work?

@annevk
Copy link
Member

annevk commented Jul 29, 2021

It seems at that point they should return early. It's probably worth sharing that code with them though when making such a request. 😊

@jakearchibald
Copy link
Collaborator Author

Just to double check, you don't think that code should go in a wpt?

@annevk
Copy link
Member

annevk commented Jul 29, 2021

I'd be okay with adding a modified version of that code to WPT, e.g., to test that Content-Type doesn't get added and POST to a data: URL works, but it would have to be in line with the requirements in the specification.

@jakearchibald
Copy link
Collaborator Author

Test PR web-platform-tests/wpt#29849

@jakearchibald
Copy link
Collaborator Author

Test PR merged

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 6, 2021
…etects, a=testonly

Automatic update from web-platform-tests
Add tests for streaming upload feature detects (#29849)

Discussed in whatwg/fetch#1275
--

wpt-commits: fe6e8e875bc09f5f37bb610c81a698ee53c71739
wpt-pr: 29849
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants