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

respose.setHeader() should return response to allow chaining #33148

Closed
SrBrahma opened this issue Apr 29, 2020 · 5 comments · Fixed by #35924
Closed

respose.setHeader() should return response to allow chaining #33148

SrBrahma opened this issue Apr 29, 2020 · 5 comments · Fixed by #35924
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@SrBrahma
Copy link

Currently, response.setHeader() doesn't return anything. It really should simply return the response to allow chainings like response.setHeader(xxx).send(yyy)

@ttsugriy
Copy link

this can be easily accomplished by adding return this to https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L550 but it would make usage of this function inconsistent with the rest of the setter APIs. While the pattern you're referring to is very common and is also known as builder, technically it violates the command-query separation principle. If Node maintainers don't mind this change, I'd be happy to send a PR :)

@himself65 himself65 added the feature request Issues that request new features to be added to Node.js. label Apr 30, 2020
@awwright
Copy link
Contributor

awwright commented Jun 8, 2020

Is this related to #25935?

However, I would like to discourage this, because of the command-query separation principle; which is to say, methods that return things should not also mutate the state of the object.

@SrBrahma
Copy link
Author

SrBrahma commented Jun 8, 2020

Is this related to #25935?

However, I would like to discourage this, because of the command-query separation principle; which is to say, methods that return things should not also mutate the state of the object.

Yes, looks the same issue. However, I really don't understand why this principle should be followed in this particular case. In the case that I mentioned,

response.setHeader(xxx).send(yyy)

as chaining isn't allowed, must currently be written as

response.setHeader(xxx)
response.send(yyy)

Chaining is a natural thing in JS. Wouldn't code like [1,2,3].map(...).filter(...).reduce(...) also violate this "principle"? For me, it only reduces productivity and readability having to follow something that doesn't make too much sense in this language.

This may be closed as duplicate, if wanted.

@ttsugriy
Copy link

ttsugriy commented Jun 8, 2020

@SrBrahma, I'm not a contributor to nodejs project, but I can answer your question as to if [1,2,3].map(...).filter(...).reduce(...) does or does not present the same API issue. The answer is no, because each call returns a new list instead of modifying an existing one, so all of these calls are "queries".

@awwright
Copy link
Contributor

awwright commented Jun 10, 2020

Chaining is a natural thing in JS

@SrBrahma I would disagree about "natural". It's tolerated; and I know some places do promote it; but there's also a strong tradition for functional programming, and when I see code like response.setHeader(xxx).send(yyy) my first thought is "Ah, setHeader must be a pure function, because it has a return value; and send must be a mutating function because the return value isn't used for anything".

The fact it's named setHeader does not suggest it's a mutating function; this naming scheme often suggests it returns a copy of response except with one thing changed.

@Trott Trott closed this as completed in dedd061 Nov 30, 2020
danielleadams pushed a commit that referenced this issue Dec 7, 2020
Make `response.setHeader` return the response object itself
so that multiple header setting can be chained.

Fixes: #33148

PR-URL: #35924
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this issue Dec 8, 2020
Make `response.setHeader` return the response object itself
so that multiple header setting can be chained.

Fixes: nodejs#33148

PR-URL: nodejs#35924
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue May 1, 2021
Make `response.setHeader` return the response object itself
so that multiple header setting can be chained.

Fixes: #33148

PR-URL: #35924
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants