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 used body replacement test for Request constructor #9931

Conversation

Projects
None yet
5 participants
@harrishancock
Copy link
Contributor

harrishancock commented Mar 8, 2018

This is a WPT companion PR for a Fetch spec change: whatwg/fetch#675. That change allows this currently throwy code snippet to function as expected:

let request = new Request(url, { method: "POST", body: "foo" })
await request.text()  // disturb the body

// currently throws because request is disturbed,
// even thought we're providing a new body
request = new Request(request, { body: "bar" })

I tested this on Chrome and Epiphany Tech Preview. As expected, it fails on both because: Chrome does not implement request.body, and EPT implements the spec as currently written.

/cc @yutakahirano, @annevk

@wpt-pr-bot wpt-pr-bot added the fetch label Mar 8, 2018

@wpt-pr-bot wpt-pr-bot requested review from annevk, jdm, mnot, youennf and yutakahirano Mar 8, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 8, 2018

Build PASSED

Started: 2018-03-09 19:52:17
Finished: 2018-03-09 20:12:23

View more information about this build on:

const bodyReplaced = new Request(bodyConsumed, { body: "Replaced body" });
assert_not_equals(bodyReplaced.body, originalBody, "new request's body is new");
assert_false(bodyReplaced.bodyUsed, "bodyUsed is false when request is not disturbed");
return bodyReplaced.text(text => {

This comment has been minimized.

Copy link
@yutakahirano

yutakahirano Mar 8, 2018

Contributor

return bodyReplaced.text().then(text => {...});

This comment has been minimized.

Copy link
@harrishancock

harrishancock Mar 8, 2018

Author Contributor

Whoa! You're right, will fix. There are multiple other instances of this in the other tests in this file -- will fix those too while I'm at it.

This comment has been minimized.

Copy link
@harrishancock

harrishancock Mar 9, 2018

Author Contributor

Done.

harrishancock added some commits Mar 8, 2018

Add used body replacement test for Request constructor
This tests the changes in Fetch spec's PR #675. Specifically, that PR
allows Requests with disturbed bodies to be used as the first parameter
to the Request constructor, as long as the RequestInit dictionary
contains a body member with which to replace the used body.

As of this writing, no browser implements this change.
Fix use of request.text() in disturbed request tests
request.text() returns a promise, which we should .then().

@harrishancock harrishancock force-pushed the harrishancock:harrishancock/fetch-pr-675-test-body-replacement-of-disturbed-request branch from a5624a8 to 1d63d7e Mar 9, 2018

@annevk

annevk approved these changes Mar 15, 2018

@annevk annevk merged commit e87f380 into web-platform-tests:master Mar 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

annevk added a commit to whatwg/fetch that referenced this pull request Mar 17, 2018

Allow used body replacement in Request constructor
Currently, the following code snippet replaces request's body as one
would expect:

  let request = new Request(url, { method: "POST", body: "foo" })
  request = new Request(request, { body: "bar" })

But this snippet throws a TypeError early in Request's constructor:

  let request = new Request(url, { method: "POST", body: "foo" })
  await request.text()  // disturb the body
  request = new Request(request, { body: "bar" })  // throws

This commit's changes allows the latter code snippet to work like the
first one.

Tests: web-platform-tests/wpt#9931.

Fixes #674.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.