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

fetch: multiple small fixes #1166

Merged
merged 2 commits into from
Jan 13, 2022
Merged

fetch: multiple small fixes #1166

merged 2 commits into from
Jan 13, 2022

Conversation

KhafraDev
Copy link
Member

This PR contains 3 fetch bug fixes:

  • In mainFetch, response was not being set in 2 places ("then set response to a network error.").
  • In httpRedirectFetch, the old step 12 has been removed
  • The terms "same origin" actually refers to a series of steps that need to be taken, not just comparing the URL's origin. (see here)

@KhafraDev KhafraDev changed the title Fetch main fetch bug fixes fetch: multiple small fixes Jan 12, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2022

Codecov Report

Merging #1166 (b299bff) into main (f569c49) will increase coverage by 0.02%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1166      +/-   ##
==========================================
+ Coverage   93.76%   93.79%   +0.02%     
==========================================
  Files          40       40              
  Lines        3817     3818       +1     
==========================================
+ Hits         3579     3581       +2     
+ Misses        238      237       -1     
Impacted Files Coverage Δ
lib/fetch/index.js 78.04% <0.00%> (+0.12%) ⬆️
lib/fetch/request.js 81.74% <ø> (ø)
lib/fetch/util.js 75.67% <75.00%> (+1.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f569c49...b299bff. Read the comment docs.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work!

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ronag ronag merged commit 9bbeba7 into nodejs:main Jan 13, 2022
@KhafraDev KhafraDev deleted the fetch-mainFetch-bug-fixes branch January 13, 2022 15:35
KhafraDev added a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
* fix: 'same origin' & setting response

* fix: remove httpRedirectFetch step #12
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022
* fix: 'same origin' & setting response

* fix: remove httpRedirectFetch step #12
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* fix: 'same origin' & setting response

* fix: remove httpRedirectFetch step nodejs#12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants