Skip to content

Conversation

@GertSallaerts
Copy link
Contributor

Recent changes in #1554 cause the response body to not be decoded when a Location header is set and the request's redirect option is set to follow.

For us this caused a few problems because we set the Location header on 201 responses to reference the location of the created resource. When looking at the spec, I don't think we're doing anything wrong on our end here.

For 201 (Created) responses, the Location value refers to the primary resource created by the request. For 3xx (Redirection) responses, the Location value refers to the preferred target resource for automatically redirecting the request.

https://www.rfc-editor.org/rfc/rfc9110.html#name-location

I updated the code to only skip decoding when the response code matches one of the codes defined as a redirectStatus as that's also what decides whether or not fetch is going to follow the redirect or not.

@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2022

Codecov Report

Merging #1628 (82f1f8e) into main (89a340f) will increase coverage by 0.72%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1628      +/-   ##
==========================================
+ Coverage   94.93%   95.66%   +0.72%     
==========================================
  Files          50       37      -13     
  Lines        4799     2838    -1961     
==========================================
- Hits         4556     2715    -1841     
+ Misses        243      123     -120     
Impacted Files Coverage Δ
index.js 81.57% <0.00%> (-18.43%) ⬇️
lib/core/util.js 88.60% <0.00%> (-8.87%) ⬇️
lib/api/readable.js 82.75% <0.00%> (-8.63%) ⬇️
lib/handler/redirect.js 83.54% <0.00%> (-7.60%) ⬇️
lib/proxy-agent.js 87.30% <0.00%> (-6.35%) ⬇️
lib/core/request.js 92.54% <0.00%> (-4.35%) ⬇️
lib/mock/mock-utils.js 96.61% <0.00%> (-3.39%) ⬇️
lib/core/connect.js 98.27% <0.00%> (-1.73%) ⬇️
lib/fetch/index.js
lib/fetch/response.js
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

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