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

[Spec] Should check body _source_ on redirect #866

Merged
merged 4 commits into from Jun 10, 2020
Merged

[Spec] Should check body _source_ on redirect #866

merged 4 commits into from Jun 10, 2020

Conversation

tinovyatkin
Copy link
Member

@tinovyatkin tinovyatkin commented Jun 5, 2020

What is the purpose of this pull request?

  • Bug fix

While working on #210 I went into a particular interpretation of fetch redirect step 9.

fetch standard:

If actualResponse’s status is not 303, request’s body is non-null, and request’s body’s source is null, then return a network error.

However, our source instead of body source is checking created Request.body. This PR fixes that fact.
This is also important due to fact that we may (want) convert the body to a stream internally, so, we actually must check body source here.

In addition, well-outdated test dependency resumer@0.0.0 was replaced with native Readable.from.

In addition, while doing the above replacement, the hacky test for Buffer.concat failure was replaced with the correct implementation of the stream that can't be concatenated into a buffer.

This PR also bumps Node.JS minimum version to 10.17 from 10.16 (will also be required for #603 and for #210).

@tinovyatkin tinovyatkin changed the title correct-stream-tests [Spec] Should check body _source_ on redirect Jun 5, 2020
@tinovyatkin tinovyatkin added the bug label Jun 5, 2020
@xxczaki
Copy link
Member

xxczaki commented Jun 9, 2020

@node-fetch/reviewers Could you please take a look at this PR?

@larson-carter
Copy link
Member

I just have a few questions as to why we are bumping up the dependencies?

@tinovyatkin tinovyatkin mentioned this pull request Jun 10, 2020
35 tasks
@Richienb
Copy link
Member

@tinovyatkin @larson-carter wants to know why the minimum version of Node.js is being increased.

@JefferyHus
Copy link
Member

JefferyHus commented Jun 10, 2020

@tinovyatkin @larson-carter wants to know why the minimum version of Node.js is being increased.

He already answered this question.

To allow use of Readable.from. It will be also required for the new version of fetch-blob and #603 and #210.

@tinovyatkin tinovyatkin merged commit 1cb9070 into node-fetch:master Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants