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.

Sign up for GitHub

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 to error handling mechanism: fail only on promise rejections #2196

Merged
merged 4 commits into from
Jan 5, 2018
Merged

Fix to error handling mechanism: fail only on promise rejections #2196

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd0ffe3
Fix to error handling mechanism: fail only on promise rejections
kravets-levko Jan 2, 2018
effb212
Fix to error handling: CR1
kravets-levko Jan 3, 2018
aa970a0
Fix to error handling: CR2
kravets-levko Jan 3, 2018
91d29cb
Fix to error handling: CR3
kravets-levko Jan 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions client/app/components/app-view/error-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,17 @@ export class ErrorHandler {
}

process(error) {
this.reset();
if (!(error instanceof Error)) {
if (error.status && error.data) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

// $q rejection
switch (error.status) {
case 403: error = new Error(''); break;
case 403: error = new Error('You have no permissions to view this page.'); break;
default: error = new Error(error.data.message); break;
}
}
this.error = error;
}
this.error = error;
if (this.logToConsole) {
// eslint-disable-next-line no-console
console.error(error);
Expand Down