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

Reply send Method Does Not Set 204 Status #263

Closed
wooliet opened this issue Sep 20, 2017 · 3 comments
Closed

Reply send Method Does Not Set 204 Status #263

wooliet opened this issue Sep 20, 2017 · 3 comments
Labels
bug Confirmed bug help wanted Help the community by contributing to this issue

Comments

@wooliet
Copy link

wooliet commented Sep 20, 2017

Documentation for Reply states that not sending any content results in 204 status response.

https://github.com/fastify/fastify/blob/master/docs/Reply.md#code

If not settled via reply.code, the resulting statusCode will be 200 or 204 if there is not content to send.

I don't think this is the case, but I might be missing something.

reply.js #37

if (payload === undefined) {
  if (!this.res.statusCode) {
    this.res.statusCode = 204
  }

From what I can tell, res.statusCode will already be set to 200 (its default value). So calling reply.send() will send status 200 and not 204.

If the above is correct, I believe the relevant portion of the documentation should be removed.

@mcollina
Copy link
Member

Good spot! I tend to say the code is assuming too much and it should not attach a status code. Would you mind sending a PR?

@mcollina mcollina added the bug Confirmed bug label Sep 20, 2017
@mcollina mcollina added this to TODO in Roadmap to 1.0 Sep 20, 2017
@mcollina mcollina added the help wanted Help the community by contributing to this issue label Sep 20, 2017
@nileshmali
Copy link
Contributor

nileshmali commented Sep 20, 2017

@mcollina You mean we should remove below block and update docs?

if (!this.res.statusCode) {
  this.res.statusCode = 204
}

@mcollina
Copy link
Member

Yes.

@nileshmali nileshmali mentioned this issue Sep 20, 2017
delvedor added a commit that referenced this issue Sep 20, 2017
@mcollina mcollina moved this from TODO to DONE in Roadmap to 1.0 Sep 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug help wanted Help the community by contributing to this issue
Projects
No open projects
Development

No branches or pull requests

3 participants