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

Never mutate user errors #154

Closed
wants to merge 1 commit into from

Conversation

frenzzy
Copy link
Member

@frenzzy frenzzy commented May 17, 2018

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
Copy link

codecov-io commented May 17, 2018

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%> (ø) ⬆️

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.

@frenzzy frenzzy force-pushed the do-not-mutate-error branch from 7766d92 to 5b99654 Compare May 18, 2018 07:02
@frenzzy frenzzy force-pushed the do-not-mutate-error branch from 5b99654 to 5ec27fb Compare August 23, 2018 11:11
@frenzzy frenzzy force-pushed the do-not-mutate-error branch from 5ec27fb to f5d8863 Compare August 28, 2018 17:56
@gpoitch
Copy link

gpoitch commented Oct 3, 2018

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
Copy link
Member Author

frenzzy commented Oct 3, 2018

@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
Copy link

gpoitch commented Oct 3, 2018

@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 frenzzy mentioned this pull request Oct 5, 2018
9 tasks
@frenzzy
Copy link
Member Author

frenzzy commented Oct 5, 2018

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

@frenzzy frenzzy closed this in #158 Oct 11, 2018
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.

5 participants