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

proposal: consistent error handling with Ucanto.Result #7

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

travis
Copy link
Member

@travis travis commented Jan 25, 2024

@alanshaw @Gozala and I had a quick huddle about Ucanto.Result inspired by a discussion in w3infra (storacha/w3infra#318 (comment)) and I committed to writing up a proposal describing the tradeoffs of using only Result for error handling vs using Result plus normal JavaScript error handling.

I've proposed we standardize on Result purity and offered to take a pass through the code to ensure we are wrapping all possible thrown errors in error Results but want to get feedback from the team before I do that!

Comments sincerely requested - I, for one, could pretty easily be swayed to the "error Results for known errors, thrown errors for unexpected stuff" camp, so if you feel strongly please let us know!

We had a quick huddle about `Ucanto.Result` inspired by a discussion in `w3infra` (storacha/w3infra#318 (comment)) and I committed to writing up a proposal describing the tradeoffs of using _only_ `Result` for error handling vs using `Result` plus normal JavaScript error handling.

I've proposed we standardize on `Result` purity and offered to take a pass through the code to ensure we are wrapping all possible thrown errors in error `Result`s but want to get feedback from the team before I do that!

Comments sincerely requested - I, for one, could pretty easily be swayed to the "error `Result`s for known errors, thrown errors for unexpected stuff" camp, so if you feel strongly please let us know!
@travis
Copy link
Member Author

travis commented Jan 25, 2024

Rendered HTML link for your perusing pleasure here: https://github.com/web3-storage/RFC/blob/proposal/ucanto-result/rfc/ucanto-result.md

Copy link
Contributor

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

yes! I agree, we also did that already in Filecoin and I was hoping to get it the default everywhere

rfc/ucanto-result.md Show resolved Hide resolved
rfc/ucanto-result.md Outdated Show resolved Hide resolved
rfc/ucanto-result.md Show resolved Hide resolved
@travis
Copy link
Member Author

travis commented Jan 26, 2024

I discovered an annoyance with the "Results only" error handling paradigm today: I was trying to implement a method in an interface (PlansStorage#initialize) and realized during the implementation that I want to return an error that was not specified in the interface. ie, I wanted to return a "invalid input" when the "external account ID" was not a stripe: URI:

https://github.com/web3-storage/w3infra/pull/318/files#diff-87f9a6d13813682bc1d417cd032ae3317f0cc2669d19ab5e5558f7ee48e73d1eR23

I ran into the same problem updating PlansStorage#set and opted to return not-yet-specified error types:

https://github.com/web3-storage/w3infra/pull/318/files#diff-87f9a6d13813682bc1d417cd032ae3317f0cc2669d19ab5e5558f7ee48e73d1eR66

Both of these approaches seem like reasonable short-term ways to get the code running. The second approach means I need to go back and update w3up/upload-api to specify more error results. The first gets my code typechecking, but violates the proposal above.

We could solve this by saying that interfaces like this should always accept Ucanto.Failure (ie, specify error types like type MyError = ErrorA | ErrorB | Ucanto.Error) but this means we lose some of the benefits of typing.

Exhaustively specifying possible error types in interfaces seems like the cleanest way to do this, but is annoying, especially in a case where updating an upstream interface is non-trivial (ie, because you don't control it, or something similar) and in this case I genuinely don't know what to do - can't throw, can't return an unexpected error, don't want to just eat the error...

@Gozala any thoughts on this?

travis added a commit to storacha/w3up that referenced this pull request Jan 26, 2024
Introduce `UnexpectedError` to allow implementations to return errors that weren't anticipated in the interface definition.

More information about this problem here:

storacha/RFC#7 (comment)

Also in this PR:

1) export plan.js as a module so callers can use error classes defined there
2) use `prettier` to make things prettier
travis added a commit to storacha/w3up that referenced this pull request Jan 29, 2024
Introduce `UnexpectedError` to allow implementations to return errors
that weren't anticipated in the interface definition.

More information about this problem here:

storacha/RFC#7 (comment)

Also in this PR:

1) export plan.js as a module so callers can use error classes defined
there
2) use `prettier` to make things prettier
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.

4 participants