kriasoft / universal-router

A simple middleware-style router for isomorphic JavaScript web apps
https://www.kriasoft.com/universal-router/
MIT License
1.7k stars 104 forks source link

Never mutate user errors #154

Closed frenzzy closed 5 years ago

frenzzy commented 6 years ago

Some libraries freezes an error object before rejection (Object.freeze(err)). Because code which adds context and code to the error object throws an exception, the router unable to call error handler and some exceptions may become unhandled.

Solution: never mutate third-party error object.

ref #152

codecov-io commented 6 years ago

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #154   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         147    153    +6     
  Branches       44     45    +1     
=====================================
+ Hits          147    153    +6
Impacted Files Coverage Δ
src/UniversalRouter.js 100% <100%> (ø) :arrow_up:

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 494bddd...f5d8863. Read the comment docs.

gpoitch commented 5 years ago

Could there be a way to disable adding context/code to the error completely? For example, I log errors to a service and have to delete these properties before sending.

frenzzy commented 5 years ago

@gpoitch good point! I think we should not even try to change error objects. Something like #152 but it will be a breaking change. But there should be a way to inform error handler about which kind of error is happened (404 or 500).

gpoitch commented 5 years ago

@frenzzy thanks for the feedback. #152 looks like a good solution to me, and provides the context incase you want it.

code feels like something that should be handled by user code, but maybe I'm missing something. Perhaps just document that adding { path: '(.*)' } last will capture "404s" and catch will capture the "500s"

frenzzy commented 5 years ago

@gpoitch correct, I just created a PR with this change. Would be cool if you could take a look: #158