koa icon indicating copy to clipboard operation
koa copied to clipboard

deps: update http-errors to v2.0.0

Open 3imed-jaberi opened this issue 5 years ago β€’ 8 comments

  • https://github.com/jshttp/http-errors/blob/master/HISTORY.md

3imed-jaberi avatar Jul 14 '20 10:07 3imed-jaberi

Codecov Report

Merging #1486 (6ead024) into master (f3c67d9) will not change coverage. The diff coverage is n/a.

@@           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.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update f3c67d9...6ead024. Read the comment docs.

codecov[bot] avatar Jul 14 '20 10:07 codecov[bot]

@dead-horse, I liked the latest updates to be present in the package.json file as you see, but if there's a problem with that I will close all updates PRs except for #1480 πŸ˜‡.

3imed-jaberi avatar Jul 15 '20 08:07 3imed-jaberi

I will close all updates PRs except for #1480 πŸ˜‡.

Would you mind closing all minor/patch PRs except or should I do that for you?

miwnwski avatar Aug 17 '20 13:08 miwnwski

I will close all updates PRs except for #1480 πŸ˜‡.

Would you mind closing all minor/patch PRs except or should I do that for you?

Thank u @miwnwski, i will do. please can you tell me what you think here #1480

3imed-jaberi avatar Aug 19 '20 01:08 3imed-jaberi

please can you tell me what you think here #1480

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.

miwnwski avatar Aug 21 '20 17:08 miwnwski

To add to this (I'm typically the person around various things like http-errors):

  1. I am happy to perform some kind of critical patch against the previous major version of a module like http-errors if needed.
  2. There has definately been discussions around Koa regarding custom statuses and how to manage them better. What exists now (and thus breaks with the http-errors upgrade) is kind of a best effort anyway, as changing the data in statuses module is just a best hope that http-errors and others actually have the same copy of it in the node_modules tree in order for it to work.

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 statuses you're changing + the statuses http-errors uses is the same copy in node_modules in the current design.

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 http-errors will accept any error status code, even if it does not exist in statuses, I think there could be some kind of overall status management in Koa (like suggested by @miwnwski ) that ties all this together.

dougwilson avatar Aug 21 '20 17:08 dougwilson

@dougwilson thank you, I was hesitant to ping you regarding my thoughts on statuses because it felt like bikeshedding.

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.

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.

miwnwski avatar Aug 21 '20 17:08 miwnwski

@3imed-jaberi @dead-horse I think it's ready to merge, what do you think about it?

etroynov avatar Aug 29 '23 11:08 etroynov