Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1k
Comments
Changes in Response Class contructor for status range and null body status check#1706
Changes in Response Class contructor for status range and null body status check#1706asingh04 wants to merge 2 commits intonode-fetch:mainfrom
Conversation
…status in Response constructor
| 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 caseoptions.status !== undefined, so you can simply doconst 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
conststatus=options.status!=null ?validateStatusValue(options.status) :200;
if thestatus is undefined( when the client/application hasnt prvovided the value), the default value200 should be assumed, but the functionvalidateStatusValue will be throwing an error. That's why added the check forundefined 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!=. Whenoptions.status isundefined,options.status != null is falsy, and will set a default 200 to the status.
Uh oh!
There was an error while loading.Please reload this page.
| 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.
Uh oh!
There was an error while loading.Please reload this page.
| 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 need205 and304, as perthe standard
Co-authored-by: Jason Zhang <xzha4350@gmail.com>
asingh04 commentedApr 9, 2023
Hi@jazelly i will be re-working on this PR as I have mis-implemented the |
Uh oh!
There was an error while loading.Please reload this page.
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.