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

Adding a headers setter to match the request api in the response api #1237

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MYDIH
Copy link

@MYDIH MYDIH commented Aug 31, 2018

Hi,

In a matter of consistency, I wish to be able to use a headers setter on the response, like I would on a request.

Thanks

PS: Koa is amazing, thank you guys

@codecov
Copy link

codecov bot commented Aug 31, 2018

Codecov Report

Merging #1237 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1237   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files           5       5           
  Lines         391     392    +1     
======================================
+ Hits          391     392    +1
Impacted Files Coverage Δ
lib/response.js 100% <100%> (ø) ⬆️

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 2180839...b91d595. Read the comment docs.

@dead-horse
Copy link
Member

-0 for me, I'd not like to add a duplicate API, and most of users are already used to set header with response.set(headers)

@fl0w
Copy link
Contributor

fl0w commented Aug 31, 2018

I'm +/-0 also.

@slaskis
Copy link
Contributor

slaskis commented Aug 31, 2018

I can see how res.headers = is expected usage.

If anything maybe we can freeze the res object from properties that are not in use by koa to avoid confusion? Probably a very breaking change found at runtime sadly.

@MYDIH
Copy link
Author

MYDIH commented Aug 31, 2018

I know that adding a duplicate API seems useless, but there is already one in the Request (headers is just calling header) ... I personnaly think that consistency should be preserved between the two APIs (adding set/get methods to Request is fine too). Having this would make things more obvious ...

Copy link
Member

@3imed-jaberi 3imed-jaberi left a comment

Choose a reason for hiding this comment

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

I'm +/-0 also. But I can say this help the newcomers.

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.

None yet

5 participants