-
-
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
deps: update http-errors to v2.0.0 #1486
deps: update http-errors to v2.0.0 #1486
Conversation
3imed-jaberi
commented
Jul 14, 2020
- https://github.com/jshttp/http-errors/blob/master/HISTORY.md
Codecov Report
@@ Coverage Diff @@
## master #1486 +/- ##
=======================================
Coverage 99.60% 99.60%
=======================================
Files 5 5
Lines 507 507
Branches 142 142
=======================================
Hits 505 505
Misses 2 2 Continue to review full report at Codecov.
|
@dead-horse, |
Would you mind closing all minor/patch PRs except or should I do that for you? |
I'm not to sure what to say about it, it's a breaking change so that can't be merged unless we do a major bump. Also, as dead-horse mentioned, I agree that we need better approach to handle to custom statuses in Koa. Statuses could have an API for adding and changing statuses, however I feel that's out of @jshttp/statuses scope. Also, it's just an object which can be modified so "an API" might just be over-engineering it. Simply exporting statuses in Koa lib feels inelegant/lazy IMO. I personally feel that a @koajs/statuses should be created (which in turn depends on @jshttp/statuses) that Koa should depend on instead. That would allow for some extra convenience, but keeps complexity where it belongs. Others might disagree. |
To add to this (I'm typically the person around various things like
There was a discussion / idea about adding an API to statuses, but as @miwnwski mentioned it is likely out of scope -- besides still having the general issue where you're still hoping that the I believe a lot of this (correct me if I'm wrong) has to do with Koa rejecting replying with a status it does not know about, so folks certainly need to add custom statuses for that case as the most common on. I'm not saying that is good or bad, just I think that is the main source for the custom statuses management in Koa. Since |
@dougwilson thank you, I was hesitant to ping you regarding my thoughts on statuses because it felt like bikeshedding.
That's an interesting observation. With that in mind, one could imagine a flag to force or not the existence of a status code. But I still think that's an implementation that probably lives better in separate Koa-centric statuses package. I'll draft up a PR and a repo on Sunday and ping you for feedback unless somebody beats me to it. |
a33ddec
to
6b9cc53
Compare
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.
Updated to the latest release v2.0.0 ✅
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.
#1480 🚨
@3imed-jaberi @dead-horse I think it's ready to merge, what do you think about it? |
8c0c130
to
8d9b273
Compare
8d9b273
to
0b3e7b6
Compare
describe('ctx.throw(err, status)', () => { | ||
it('should throw the error and set .status', () => { | ||
const ctx = context() | ||
const error = new Error('test') | ||
|
||
try { | ||
ctx.throw(error, 422) | ||
} catch (err) { | ||
assert.strictEqual(err.status, 422) | ||
assert.strictEqual(err.message, 'test') | ||
assert.strictEqual(err.expose, true) | ||
} | ||
}) | ||
}) | ||
|
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.
@3imed-jaberi thank you! |