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

Simplify error type #132

Closed
wants to merge 3 commits into from
Closed

Simplify error type #132

wants to merge 3 commits into from

Conversation

diogob
Copy link
Collaborator

@diogob diogob commented Feb 23, 2024

This PR brings a breaking change proposed for the next major version.
It drops the optional exception property present in ErrorWithMessage in favour of always using an Error type to represent errors.

The Error already carries a message, hence once the exception becomes mandatory the wrapping object becomes entirely redundant.
Moreover, having an optional exception does not make a lot of sense since the only way to return a result with the errors property is by using exceptions.

The code-base gets a bit tidier and checking for exception types becomes simpler. There is also didactic value since every TS developer should already be familiar with Error.

  • Ensure our ErrorWithMessage type always has an attched exception
  • Simplify ErrorWithMessage so it's just an Error alias

…e might as well remove the alias and work with the native Error
@gustavoguichard
Copy link
Collaborator

I need to think through it. My main concern is how easy is it to work with at the UI level, are we gonna have helpers to extract the messages? Also, how does it affect InputErrors, EnvironmentErrors, and other abstractions people may be using.

@diogob
Copy link
Collaborator Author

diogob commented Feb 26, 2024

Using some examples from a real code base:

Expressions relying on messages would be unchanged, such as

errorData.errors.some(({ message }) => message.includes('team_name_key'))

This works since the property message exists both in our existing ErrorWithMessage and in the native Error.
We wouldn't need any helper for that, although that might be an interesting idea to tighten the abstraction.

The expression

Array.isArray(error.errors)

Would also be unchanged, or anything using the list interface.

The breaking changes happen on code that deals with the exception property, such as

'data' in error.errors[0].exception

Would have to be rewritten as

'data' in error.errors[0]

and

result.errors.find((e) => e.exception instanceof UnauthorizedError),

Would have to be rewritten as

result.errors.find((e) => e instanceof UnauthorizedError),

@diogob
Copy link
Collaborator Author

diogob commented Feb 26, 2024

InputErrors and EnvironmentErrors will no be affected as far as I can tell, since they deal with SchemaError.

@diogob
Copy link
Collaborator Author

diogob commented Feb 26, 2024

A smoother approach would be to make exception mandatory and create a function to access the exception (and perhaps the message). This would make for a tighter abstraction and easier future changes.

@gustavoguichard
Copy link
Collaborator

Ok, I think I like it, I've been dealing with errors[i].exception a lot lately anyway

@diogob diogob closed this May 17, 2024
@diogob
Copy link
Collaborator Author

diogob commented May 17, 2024

composable-functions is upon us!

@diogob diogob deleted the simplify-error-type branch May 20, 2024 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants