Skip to content

don't coerce header value to string on set #1394

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

Closed
wants to merge 4 commits into from
Closed

don't coerce header value to string on set #1394

wants to merge 4 commits into from

Conversation

tinovyatkin
Copy link
Contributor

@tinovyatkin tinovyatkin commented Oct 13, 2019

Official Koa documentation about response.set says This delegates to setHeader which sets or updates headers, but setHeader doesn't convert header value types until sending them to the network. This PR fixes behavior of response.set to be according to the documentation.
Also removes recursion on .set calling .setHeader directly, for performance reason.

@codecov
Copy link

codecov bot commented Oct 13, 2019

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.38%. Comparing base (9ee6584) to head (6983519).
Report is 163 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1394      +/-   ##
==========================================
- Coverage   99.38%   99.38%   -0.01%     
==========================================
  Files           4        4              
  Lines         485      484       -1     
  Branches      132      130       -2     
==========================================
- Hits          482      481       -1     
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dead-horse
Copy link
Member

This change is reasonable but may contains breaking change, I'd like to merge it when we start next major version's development.

@tinovyatkin
Copy link
Contributor Author

@dead-horse remember that in current implementation you have two more documented ways to set a response header:

ctx.res.setHeader('Content-Length', 256);
ctx.response.header['content-length'] = 256;

I've linked above that official documentation clearly states that ctx.response.set delegates to ctx.res.setHeader while in fact, it's not exactly the same. So, I don't understand why do you think it's a breaking change while, according to official documentation of Koa, it should be a bug fix.

@dougwilson
Copy link
Contributor

The current major has been out and in use long enough that at this point the bug is in the documentation for it. Seems reasonable to adjust it in next major as @dead-horse said.

@anlexN
Copy link

anlexN commented Aug 27, 2020

good idea.
@dead-horse can we make koa v3 branch to develop?
@tinovyatkin can we together do it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants