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

feat: Allow non 204 status for null JSON responses #1421

Closed
wants to merge 1 commit into from
Closed

feat: Allow non 204 status for null JSON responses #1421

wants to merge 1 commit into from

Conversation

pke
Copy link

@pke pke commented Nov 21, 2019

Introduces a new application option emptyBodyAs204 that must be set explictly to false when creating the Koa app to prevent Koa from treating null body as 204.

Set it via

new Koa({
  response: {
    emptyBodyAs204: false
  }
});

Closes #998

I'll provide documentation updates when we decided on a final name.

Introduces a new application option `emptyBodyAs204` that must be set explictly to `false` when creating the Koa app to prevent Koa from treating `null` body as 204.

Set it via

```js
new Koa({
  response: {
    emptyBodyAs204: false
  }
});
```
Closes #998
@codecov
Copy link

codecov bot commented Nov 21, 2019

Codecov Report

Merging #1421 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1421      +/-   ##
==========================================
+ Coverage   99.37%   99.38%   +<.01%     
==========================================
  Files           4        4              
  Lines         479      485       +6     
  Branches      128      129       +1     
==========================================
+ Hits          476      482       +6     
  Partials        3        3
Impacted Files Coverage Δ
lib/response.js 99.38% <100%> (+0.01%) ⬆️
lib/application.js 98.26% <100%> (+0.03%) ⬆️

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 ed84ee5...3dd0627. Read the comment docs.

@jonathanong
Copy link
Member

why not set the status after setting body = null?

@yorkie
Copy link
Member

yorkie commented Nov 25, 2019

It's reasonable to use 204 if the body is null, because it does mean no-content, I guess what @damianobarbati want is a option to disable the automatic reset on .status?

@damianobarbati
Copy link

@jonathanong @yorkie I'm not sure 204 is correct: there's a body because there's a payload, a json-serializable payload.
The payload holds the content null (not undefined).
The response of the API call is null. Nothing more, no object wrapping.

I'm not sure whether you are understanding what I mean 🤔

@yorkie
Copy link
Member

yorkie commented Nov 25, 2019

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/null

The value null represents the intentional absence of any object value.

I think the primitive null means there is no content(absence) on the body, if someone wants to respond the "null", I think ctx.body = 'null' is necessary to specify it's a string.

@damianobarbati as I mentioned before, how about adding an option to disable the automatic status reset globally, which will make the status will not be set to 204 when you set the body to null, and it might be more useful usually.

@pke
Copy link
Author

pke commented Nov 26, 2019

@damianobarbati as I mentioned before, how about adding an option to disable the automatic status reset globally, which will make the status will not be set to 204 when you set the body to null, and it might be more useful usually.

That's what this PR provides, isn't it?

@yorkie
Copy link
Member

yorkie commented Nov 27, 2019

This PR is just adding an option for a specific case

@dead-horse
Copy link
Member

I don't think add this option is a good idea, it is easy to implement a middleware to handle this.

(ctx, next) => {
  await next();
  if (ctx.body === null) {
    ctx.body = 'null';
  }
}

@pke
Copy link
Author

pke commented Dec 4, 2019

Great idea! I think that should work for you @damianobarbati?

@pke pke closed this Dec 4, 2019
@pke pke deleted the feature/allow-null-body-for-json-responses branch December 4, 2019 14:01
@damianobarbati
Copy link

@pke @dead-horse thanks but I did already try before opening the issue a very long time ago and I had no way to overcome the 204: I suspect Koa is doing something when body is null preventing me from setting any body.

In the following snippet am I doing something wrong? Is it supposed to work?
And I guess I have to set the content-type manually too, right..? 😔

import koa from 'koa';
const app = new koa();

app.use(async (ctx, next) => {
    await next();

    if (ctx.body === null) {
        ctx.set('Content-Type', 'application/json; charset=utf-8');
        ctx.body = 'null';
    }
});

app.use(async ctx => ctx.body = null);

app.listen(80)

@damianobarbati
Copy link

@yorkie yes I think an option to prevent the aforementioned (default) behaviour of Koa would work.
My concern isn't only about the inability to provide a null json, I guess it's rather a philosophical concern.

The 204 on json payload of null looks like a (smart) bias to me.
It's definitely the best move because typically when a you set body to null you are returning... nothing.
But here the payload is not null: the payload is a JSON string.
The value expressed by the payload is the primitive null.

It's ambiguous when it comes to API request and response:

  • consumer request the content of something
  • producer response the content is null

But the producer must return a message (json string) expressing the content is null (null value encoded) to consumer.
The consumer shouldn't be inferring the content (null) from the http headers (204), because it expects a payload in the agreed format (json) expressing null (string) and not an empty response.

Koa has a right default behaviour covering most of cases, still I think an opt-out should be given to developers.

But I'm just discussing/brainstorming guys! It's not my intention to start a flame 😅

  • community didn't raise this so far and I can find some kind of workaround
  • koa is great and your work is amazing 😈

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.

Empty response if result is null
5 participants