Skip to content

FabricErrorHandler makes Angular app super crashable #134

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

Closed
hosikiti opened this issue Aug 4, 2018 · 3 comments
Closed

FabricErrorHandler makes Angular app super crashable #134

hosikiti opened this issue Aug 4, 2018 · 3 comments
Assignees
Labels

Comments

@hosikiti
Copy link

hosikiti commented Aug 4, 2018

If I use FabricModule.forRoot() , angular app crashes super easily.
For example, following TypeScript codes will cause JavaScript error.

let x: any = {};
let msg = x.y.getMessage(); // This line causes -> TypeError: undefined is not an object (evaluating 'x.y.getMessage')

Without using FabricModule.forRoot(), the line shows JavaScript errors on console but apps do NOT crash. However if you use FabricModule.forRoot(), the line causes instant app freeze and crash.

It seems following line of errorhandler.ts causes app crash:

If I commented out throw err;, apps stop crashing.

In development, this crash behavior is acceptable. But in production, app crash can seriously hurt your app's credibility. So do you think you could replace throw err; with simple console output something like following:

console.error(err, err.stack);
@hypery2k
Copy link
Owner

hypery2k commented Aug 5, 2018

this behaviour is intentional: An Angular Error Handler should catch errors, handle it with his own logic and rethrow it. Why should you use an ErrorHandler if you don't want to handle it?

@hypery2k hypery2k self-assigned this Aug 5, 2018
@hypery2k hypery2k closed this as completed Aug 5, 2018
@hosikiti
Copy link
Author

hosikiti commented Aug 5, 2018

@hypery2k If this behaviour is intentional as you said, I think it should be documented somewhere. Although I wanted to handle errors, I didn't expect that adding this ErrorHandler would cause unexpected crash in production. Currently adding the ErrorHandler is documented as if it is setup procedure. If I had known the side-effect of FabricModule.forRoot(), I would not use it.

hypery2k added a commit that referenced this issue Aug 5, 2018
BREAKING CHANGE: Error Handler will not rethrow error!!
@hypery2k
Copy link
Owner

hypery2k commented Aug 5, 2018

got it, would like to make it configurable, but not having time for this. I removed the rethrow. If the CI build went through, you can give the @next version a try.

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

No branches or pull requests

2 participants