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

Allow passing a Request instance to Request constructor #179

Merged
merged 4 commits into from
Jul 31, 2015

Conversation

mislav
Copy link
Contributor

@mislav mislav commented Jul 19, 2015

I tried to match the spec as closely as possible. This should also make it possible to use fetch(request, options) invocation.

Fixes #177

/cc @dgraham @cesarandreu

@cesarandreu
Copy link

Oh awesome! That's was really fast.

@cesarandreu
Copy link

I just looked through the spec and comparing it with Chrome Canary they seem to at least behave the same.

The new Request instance becomes essentially a clone of the old Request.
The original Request's body becomes unusable.

https://fetch.spec.whatwg.org/#dom-request
This makes the test error more correct when the `then` block raises an
assertion error. The `catch` block needs to re-raise the same exception,
not swallow it and disguise it like another failure.
@mislav
Copy link
Contributor Author

mislav commented Jul 20, 2015

We got a legitimate failure in Chrome because it doesn't conform to the spec fully: a Request with an already consumed body should throw an exception, and in Chrome it doesn't. @dgraham What do we do in these situations?

@dgraham
Copy link
Contributor

dgraham commented Jul 21, 2015

@mislav I usually test the behavior in Firefox as well, to determine if both browsers agree on the implementation. It could be a misreading of the specification or a real Chrome bug.

@mislav
Copy link
Contributor Author

mislav commented Jul 21, 2015

It passes in Firefox, as well as my polyfill implementation based on the spec. It's not a super-important feature, though— it just throws a TypeError early when the body has already been consumed. It looks like Chrome implementation in general allows bodies to be re-consumed.

@cesarandreu
Copy link

Possibly related to the following?
whatwg/fetch#55

@mislav
Copy link
Contributor Author

mislav commented Jul 27, 2015

Thanks @cesarandreu.

@dgraham It's a real Chrome bug due to the spec only formalizing this very recently. Should I skip the test in Chrome?

@dgraham
Copy link
Contributor

dgraham commented Jul 27, 2015

@mislav That sounds fine. We can re-enable the test after Chrome ships a fix for the bug.

The fetch spec instructs that Request should throw a TypeError when
given a Request with an already consumed body, but Chrome's
implementation doesn't respect that yet.
mislav added a commit that referenced this pull request Jul 31, 2015
Allow passing a Request instance to Request constructor
@mislav mislav merged commit d16df66 into master Jul 31, 2015
@mislav mislav deleted the request-to-request branch July 31, 2015 17:32
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect Request implementation
3 participants