-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add support for custom statuses #1119
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,18 @@ module.exports = class Application extends Emitter { | |
this.context = Object.create(context); | ||
this.request = Object.create(request); | ||
this.response = Object.create(response); | ||
this.customStatuses = {}; | ||
} | ||
|
||
/** | ||
* Add custom status code | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All other comments have a period after the description text. |
||
* @param {Number} code | ||
* @param {String} description | ||
* @api public | ||
*/ | ||
|
||
addStatus(code, description) { | ||
this.customStatuses[code] = description; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. only issue is naming. i'd prefer this to be called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. other than that, i'm good with this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that my opinion matters, but personally I'd prefer |
||
} | ||
|
||
/** | ||
|
@@ -166,6 +178,7 @@ module.exports = class Application extends Emitter { | |
request.ctx = response.ctx = context; | ||
request.response = response; | ||
response.request = request; | ||
response.customStatuses = this.customStatuses; | ||
context.originalUrl = request.originalUrl = req.url; | ||
context.cookies = new Cookies(req, res, { | ||
keys: this.keys, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,11 +81,11 @@ module.exports = { | |
|
||
set status(code) { | ||
assert('number' == typeof code, 'status code must be a number'); | ||
assert(statuses[code], `invalid status code: ${code}`); | ||
assert(statuses[code] || this.customStatuses[code], `invalid status code: ${code}`); | ||
assert(!this.res.headersSent, 'headers have already been sent'); | ||
this._explicitStatus = true; | ||
this.res.statusCode = code; | ||
if (this.req.httpVersionMajor < 2) this.res.statusMessage = statuses[code]; | ||
if (this.req.httpVersionMajor < 2) this.res.statusMessage = statuses[code] || this.customStatuses[code]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I get you correctly, you meant to use the custom status codes instead of the default status codes.Is that right? So, actually the logic would be:
Then, if you have no custom status codes the default status will be used instead. Is that the expected behavior? |
||
if (this.body && statuses.empty[code]) this.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.
Nit:
Object.create(null)