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

Consolidate error handling #1304

Merged
merged 7 commits into from
May 14, 2019
Merged

Consolidate error handling #1304

merged 7 commits into from
May 14, 2019

Conversation

mnzaki
Copy link
Contributor

@mnzaki mnzaki commented May 10, 2019

There's an AppError class and associated ErrorCode enum with constants for the kinds of application errors in lib/errors.ts. Usage tends to be new AppError(ErrorCode.SomeError, caughtErr). Each AppError has a related message, which it gets automatically based on type. It's a little annoying to import the class and enum and also refer to the error code like that, but it's the easiest way I could find so far. Any feedback on usage, error messages, and names of the class, enum, constants?

I tested only 2 of the error conditions (ParseJWTFailed and CredentialRequestFailed), probably needs more testing, perhaps as part of test cases for the sso actions.

The caughtError gets logged, but I also think we should introduce sentry which is coincidentally free for opensource

@mnzaki mnzaki force-pushed the fix/error_handling branch from be82d1b to 5bd6828 Compare May 10, 2019 11:01
Copy link
Contributor

@chunningham chunningham left a comment

Choose a reason for hiding this comment

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

I like how this is done, but there are still four occurrences of the old pattern in actions/account/index.ts

@mnzaki
Copy link
Contributor Author

mnzaki commented May 10, 2019

@chunningham ah missed that file, gonna convert it as well.

For the checkIdentityExists action, it's unclear what error it should throw on failure. What do you think?
I'm also thinking about removing the Failed prefix from all the enum keys :D seems redundant

@chunningham
Copy link
Contributor

theres multiple issues that can cause an identity wallet failure, mostly its internet connectivity issues but yeah something vague like "Unable to authenticate identity" or something should work. removing Failed from the enum makes sense but it is descriptive and there's not real overhead imo

@mnzaki mnzaki requested a review from chunningham May 10, 2019 14:18
mnzaki added 2 commits May 10, 2019 16:27
…/error_handling

Conflicts:
	src/actions/account/index.ts

updated the errors to match
@mnzaki mnzaki force-pushed the fix/error_handling branch from 484ba86 to 2eadf5e Compare May 10, 2019 14:30
@mnzaki
Copy link
Contributor Author

mnzaki commented May 10, 2019

Another point is about the error code strings, because we might want to actually display these to the users, and maybe they will be reported or something

Copy link
Contributor

@chunningham chunningham left a comment

Choose a reason for hiding this comment

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

nice

@chunningham chunningham merged commit ea73d64 into develop May 14, 2019
@chunningham chunningham deleted the fix/error_handling branch May 14, 2019 08:59
This was referenced May 15, 2019
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.

2 participants