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

fix: httpErrorHandler minification type checking #337

Merged
merged 5 commits into from
May 17, 2019
Merged

fix: httpErrorHandler minification type checking #337

merged 5 commits into from
May 17, 2019

Conversation

chrisandrews7
Copy link
Contributor

If the code is mangled during minification process when using webpack, the check for a type of HttpError in the httpErrorHandler middleware will fail. As the private property on super_.name will not be HttpError, the mangling would change the name.

This is due to Terser setting both keep_classnames and keep_fnames to false by default.
Either way it shouldn't really use a private property to check the type of error.

This will close #325

@codecov
Copy link

codecov bot commented May 10, 2019

Codecov Report

Merging #337 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #337   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          20     20           
  Lines         542    543    +1     
  Branches      110    110           
=====================================
+ Hits          542    543    +1
Impacted Files Coverage Δ
src/middlewares/httpErrorHandler.js 100% <100%> (ø) ⬆️

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 7fdee78...2e7cf81. Read the comment docs.

@codecov
Copy link

codecov bot commented May 10, 2019

Codecov Report

Merging #337 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #337   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          20     20           
  Lines         543    543           
  Branches      111    111           
=====================================
  Hits          543    543
Impacted Files Coverage Δ
src/middlewares/httpErrorHandler.js 100% <100%> (ø) ⬆️

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 b8af685...01409e6. Read the comment docs.

@chrisandrews7
Copy link
Contributor Author

This cant be merged until both jshttp/http-errors#56 and npm/npm#19770 have been resolved. I guess thats why it was done in the current way.

@chrisandrews7
Copy link
Contributor Author

chrisandrews7 commented May 10, 2019

Given the only thing used by the response is statusCode. Could do a check on the handler.error.statusCode property as the type identifier in the meantime? @lmammino @dkatavic @theburningmonk @vladgolubev

@lmammino lmammino self-requested a review May 17, 2019 06:58
Copy link
Member

@lmammino lmammino left a comment

Choose a reason for hiding this comment

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

Seems good, thanks @chrisandrews7!

I initially didn't like the solution to check the statusCode (I was more inclined to a check like instanceof HttError), but after thinking a bit more about it, I think it's simple enough and it might work in most cases, even if a custom middleware doesn't want to return an actual instance of HttpError.

I'll try to get this merged shortly, can you submit the same change for v1.0.0-alpha? :)

@lmammino
Copy link
Member

I actually added an extra check for message as well, added a dedicated test case and updated the docs.

I think I have enough time to forward port this to 1.0.0

@lmammino lmammino merged commit c69d858 into middyjs:master May 17, 2019
@chrisandrews7 chrisandrews7 deleted the fix/http-error-check branch May 17, 2019 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

httpErrorHandler breaks if code is minimized / mangled
2 participants