Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

3imed-jaberi
Copy link
Member

@codecov
Copy link

codecov bot commented Jul 14, 2020

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.

@3imed-jaberi
Copy link
Member Author

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

@miwnwski
Copy link
Member

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?

@3imed-jaberi
Copy link
Member Author

3imed-jaberi commented Aug 19, 2020

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

@miwnwski
Copy link
Member

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.

@dougwilson
Copy link
Contributor

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.

@miwnwski
Copy link
Member

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

Copy link
Member Author

@3imed-jaberi 3imed-jaberi left a 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 ✅

Copy link
Member Author

@3imed-jaberi 3imed-jaberi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1480 🚨

@3imed-jaberi 3imed-jaberi changed the title deps: update http-errors to v1.8.0 deps: update http-errors to v2.0.0 Apr 2, 2022
@etroynov
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants