Changes in Response Class contructor for status range and null body status check #1706
Conversation
…status in Response constructor
| @@ -23,12 +39,18 @@ export default class Response extends Body { | |||
| constructor(body = null, options = {}) { | |||
| super(body, options); | |||
|
|
|||
| // eslint-disable-next-line no-eq-null, eqeqeq, no-negated-condition | |||
| const status = options.status != null ? options.status : 200; | |||
There was a problem hiding this comment.
this check with coersion has covered the case options.status !== undefined, so you can simply do
const status = options.status != null ? validateStatusValue(options.status) : 200;
but I'm not fussed, happy with either. :)
There was a problem hiding this comment.
@jazelly in this code
const status = options.status != null ? validateStatusValue(options.status) : 200;if the status is undefined( when the client/application hasnt prvovided the value), the default value 200 should be assumed, but the function validateStatusValue will be throwing an error. That's why added the check for undefined as well.
There was a problem hiding this comment.
Yes, but notice it's a !=. When options.status is undefined, options.status != null is falsy, and will set a default 200 to the status.
| @@ -119,7 +141,8 @@ export default class Response extends Body { | |||
| } | |||
|
|
|||
| static error() { | |||
| const response = new Response(null, {status: 0, statusText: ''}); | |||
| // default error status code = 400 | |||
| const response = new Response(null, {status: 400, statusText: ''}); | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| const headers = new Headers(options.headers); | ||
|
|
||
| if (body !== null && !headers.has('Content-Type')) { | ||
| if (status === 204 && typeof body !== 'undefined' && body !== null) { |
There was a problem hiding this comment.
I think we also need 205 and 304, as per the standard
Co-authored-by: Jason Zhang <xzha4350@gmail.com>
|
Hi @jazelly i will be re-working on this PR as I have mis-implemented the |
Purpose
#1685
Changes
The validation around the input status and the validation for the null body status(204) has been added.
Additional information
npm linti found an pre-existing error in the@types/index.test-d.tsfile, where aResponseinstance was being constructed with no purpose, so i removed itpackage.jsonas"fix:lint": "xo --fix"so that trivial linting errors can be fixed automatically.Response.errormethod, the default status code was0but this will no longer be allowed due to to validation checks, so a new value400will be used as default status code.