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

Improvements around custom error handling in ServerFnError #3253

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

Conversation

nicolas-guichard
Copy link

This implements a fix for #3154 by making NoDefaultError unhinabited, and proposes workarounds for #3153 and #3155 to make converting easier:

  1. from Result<T, ServerFnError<NoCustomError>> to Result<T, ServerFnError<AnyOtherCustomError>>
  2. from some Result<T, ServerFnError<SourceCustomError>> to Result<T, ServerFnError<TargetCustomError>> when TargetCustomError: From<SourceCustomError>
  3. from Result<T, E> to Result<T, ServerFnError<AnyCustomError>> when E: std::error::Error, reflecting the existing From<E: std::error::Error> for ServerFnError<NoCustomError>

Usage examples are provided in the docstring for each of these new traits. My typical uses cases are:

  • for 1. and 2. shared helper functions returning ServerFnError already, for instance a function that calls use_context.
  • for 3. external functions that may raise an error, a database call for instance.

As mentioned in #3153 and #3155 I would have preferred implementing From<ServerFnError<SourceCustomError>> for ServerFnError<TargetCustomError> and From<E: std::error::Error> for ServerFnError, but as @benwis correctly pointed out those conflict with From<T> for T, at least until specialization becomes available.

Contrary to an empty struct, which has exactly 1 possible value, an
empty enum has 0 possible values: it can't even be constructed.

Fixes leptos-rs#3154
This adds 3 new traits:
- `ConvertServerFnResult` provides easy conversion from
  `Result<T, ServerFnError<SourceCustErr>>` to
  `Result<T, ServerFnError<TargetCustError>>` when `TargetCustError`
  implements `From<SourceCustErr>`

- `ConvertDefaultServerFnResult` provides easy conversion from
  `Result<T, ServerFnError<NoCustomError>` to
  `Result<T, ServerFnError<TargetCustError>>`

- `IntoServerFnResult` provides easy conversion from `Result<T, E>` to
  `Result<T, ServerFnError::ServerError>` when `E` implements
  `std::error::Error`

Fixes leptos-rs#3153 and leptos-rs#3155
@benwis
Copy link
Contributor

benwis commented Nov 17, 2024

I think there are some nice utilities here. Originally I had meant the server_fn_error! macro to convert Error into ServerFnError depending on the traits Error implemented. I suspect that can be expanded to handle some of these conversions here and land on a more general single trait.

@nicolas-guichard
Copy link
Author

What's the target MSRV? The CI apparently uses an old Rust nightly 2024-08-01, and the build fails because omitting empty types in pattern matching is a Rust 1.82 feature.
The question is: should this handle Rust < 1.82 by adding the missing error::ServerFnError::WrappedServerError arm in ConvertDefaultServerFnResult::convert_custom_error?

@gbj
Copy link
Collaborator

gbj commented Nov 18, 2024

We don't have an official MSRV policy but if it's simply a matter of adding a missing branch in a match, then yes you should add it.

Comment on lines 74 to +77
type Err = ();

fn from_str(_s: &str) -> Result<Self, Self::Err> {
Ok(NoCustomError)
fn from_str(_: &str) -> Result<Self, Self::Err> {
Err(())
Copy link
Contributor

Choose a reason for hiding this comment

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

Ironically, I do think Infallible would make sense here as the Err type for FromStr (since it implements Error), but this is a breaking change so not sure if its worth the effort.

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