Fix to error handling mechanism: fail only on promise rejections

[kravets-levko]

No description provided.

[arikfr]

I'm pretty sure that any $http error will have these properties... maybe we should throw an explicit error object that triggers the display?

[kravets-levko]

Aah...Yes, we can, definitely. It will be almost the same as it was in the first commit :-) But the first solution has not good architecture. Let me think a bit about it.

[arikfr]

We can utilize $routeChangeError, and then there either throw a new object that wraps the $http exception or just set the value of error.

The benefit of using the throw option, is that we can use it later in other places where we want the error to block the whole page.

[kravets-levko]

I already implemented it as custom error (we cannot use $routeChangeError because then we'll return back to a moment before I started with all that error handling - $routeChangeError sometimes will not work). Will do some tests and update this PR

[arikfr]

But $routeChangeError wasn't working because we were not rendering the component that supposed to show the error.

But with the change you did it works properly now. So you can just set the error in the $routeChangeError event handler, instead of the change in effb212.

[arikfr]

I saw the comment. The thing is you added a lot of complexity along with that error type. I don't think the added complexity is worth it. Mixing timing issues and error handling is a recipe for disaster.

We don't need to worry about the user seeing all the {{...}}, because we will show him specific errors we choose to show. TypeError isn't one of them. I'm OK with paying the price the user might see broken markup in a rare occasion if in return we get simpler code.

Also once we properly log errors the chances of a user seeing some random error are lower.

[kravets-levko]

1. Additional babel transformation is needed because without it babel cannot transpile extends Error - it will not have own class (all derived classes will be replaced with Error) and therefore will not pass instanceof checks.
2. I added wait time in order to make better user experience - it's not required, but will catch accidental errors in controller constructors (as I mentioned earlier - such errors will prevent views from rendering, so user will not see nothing). The core mechanism is built around custom error class.

[kravets-levko]

I see your comment with some delay, but got it. Will remove unnecessary code and ping you 👍

[arikfr]

babel transformation: what do we gain from extending Error instead of creating a simple class?

[kravets-levko]

I think it's just right - all errors should be derived from Error class, especially when remember that some tools like browser's console has special handling for Error class. I read a docs for babel-plugin-transform-builtin-extend - it does not add a lot of overhead - it will process only classes inherited from Error (specified in .babelrc), and will just add a simple wrapper around it to fix its behavior, that's it.

[kravets-levko]

@arikfr Updated PR. Now it works in next way: it will show all errors occurred while route is changing (between $routeChangeStart and $routeChangeSuccess/$routeChangeError), any error in grace period after $routeChangeSuccess (it allows to catch errors during controller instantiations, so user will see error instead of page with angular markup), and PromiseRejectionError - at any time (currently it is explicitly triggered in dashboard page controller in loadDashboard function). Other errors will be just logged in console.

[kravets-levko]

@arikfr Updated PR: now showing only dedicated error type (PromiseRejectionError - triggered in $routeChangeError - covers most pages, and in dashboard controller)

[arikfr]

Why it needs to be an EventEmitter? What was wrong with just setting the error value when needed to show an error?

[kravets-levko]

Yep... 👍