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
Changes in Response Class contructor for status range and null body status check #1706
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 lint
i found an pre-existing error in the@types/index.test-d.ts
file, where aResponse
instance was being constructed with no purpose, so i removed itpackage.json
as"fix:lint": "xo --fix"
so that trivial linting errors can be fixed automatically.Response.error
method, the default status code was0
but this will no longer be allowed due to to validation checks, so a new value400
will be used as default status code.