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

Fallible components #528

Open
arctic-hen7 opened this issue Nov 8, 2022 · 9 comments
Open

Fallible components #528

arctic-hen7 opened this issue Nov 8, 2022 · 9 comments
Labels
A-ergonomics Area: API ergonomics A-macro Area: macros C-enhancement Category: new feature or improvement to existing feature S-needs-design

Comments

@arctic-hen7
Copy link
Contributor

Right now, error handling in Sycamore is difficult and fairly unergonomic, since any errors have to be handled within the component in which they occur, since there is no concept yet of a component that can return an error. I would propose the following pattern:

#[fallible_component(ErrorComponent)]
fn MyComponent<G: Html>(cx: Scope, foo: String) -> Result<View<G>, MyError> {
    if foo == "bad" {
        Err(MyError::BadProp)
    } else {
        view! { cx,
            // Normal view
            ...
        }
    }
}

#[component]
fn ErrorComponent<G: Html>(cx: Scope, err: MyError) -> View<G> {
    view! { cx,
        // Error display
        ...
    }
}

Central to this is a #[fallible_component] macro or the like, which would desugar, for the above example, to something like this:

#[component(ErrorComponent)]
fn MyComponent<G: Html>(cx: Scope, foo: String) -> View<G> {
    // The user's original component, renamed
    fn __internal<G: Html>(cx: Scope, foo: String) -> Result<View<G>, MyError> {
        if foo == "bad" {
            Err(MyError::BadProp)
        } else {
            view! { cx,
                // Normal view
                ...
            }
        }
    }

    match __internal(cx, foo) {
        Ok(view) => view,
        Err(err) => view! {
            // As the user specified in the `#[fallible_component]` macro
            ErrorComponent(err)
        }
    }
}

// This is unchanged
#[component]
fn ErrorComponent<G: Html>(cx: Scope, err: MyError) -> View<G> {
    view! { cx,
        // Error display
        ...
    }
}

More formally, that macro should annotate a function with the return type Result<View<G>, E>, where the argument to the macro is a Sycamore component that takes a single argument of type E (i.e. it takes the error to process it and display it). This would allow individual components to return errors and then have them automatically handled and displayed neatly to the user.

This is just one approach, similar to the current suspense system (with its fallback view), with the obvious part lacking being allowing errors to bubble up. We could alternately allow callers to handle errors in components, though I personally don't favor this approach (I think anything that the caller should handle ought to be abstracted to a separate function that it matches on, as one would do already). That way, Sycamore provides a convenient extra rather than just formalizing the existing workaround. Notably, both approaches should be documented if the fallible components idea is accepted, since there will likely be a place for both approaches in apps.

There is also the issue of allowing errors to bubble up completely, which could be achieved with a special context element for handling errors in components. Inside that, each component could have an entry, and then other components could check as necessary if something else has failed. This would be clearly useful for parents to check if their children have failed, while sibling checks would probably only be regarding whether or not some other part of context is accessible, in which case it should probably just be in a Result itself, external to any systems Sycamore might provide.

Pending input from others, I would be very happy to work on this!

@lukechu10 lukechu10 added C-enhancement Category: new feature or improvement to existing feature A-macro Area: macros A-ergonomics Area: API ergonomics labels Nov 17, 2022
@lukechu10 lukechu10 added this to the v0.9 milestone Nov 17, 2022
@stopbystudent
Copy link

As some kind of input from others, I'd like to throw in what leptos does; the following quotes from their docs of their ErrorBoundary component:

/// When you render a `Result<_, _>` in your view, in the `Err` case it will
/// render nothing, and search up through the view tree for an `<ErrorBoundary/>`.
/// This component lets you define a fallback that should be rendered in that
/// error case, allowing you to handle errors within a section of the interface.
///
/// ```
/// # use leptos_reactive::*;
/// # use leptos_macro::*;
/// # use leptos_dom::*; use leptos::*;
/// # if false {
/// # run_scope(create_runtime(), |cx| {
/// let (value, set_value) = create_signal(cx, Ok(0));
/// let on_input = move |ev| set_value(event_target_value(&ev).parse::<i32>());
///
/// view! { cx,
///   <input type="text" on:input=on_input/>
///   <ErrorBoundary
///     fallback=move |_, _| view! { cx, <p class="error">"Enter a valid number."</p>}
///   >
///     <p>"Value is: " {value}</p>
///   </ErrorBoundary>
/// }
/// # });
/// ```

I do favor this approach instead of the one where I have to specify the error component in an attribute for two reasons:

  1. It creates looser coupling (the error handling is decoupled from throwing it) which enables logical separation into components with the main component actually handling an error.
  2. It's still sufficiently similar to the Suspense system to fit into sycamore's design.

One downside is the decision to simply not do anything at all on an error result if there is no error boundary.

I don't really know what you meant by the following section of your proposal:

We could alternately allow callers to handle errors in components, though I personally don't favor this approach (I think anything that the caller should handle ought to be abstracted to a separate function that it matches on, as one would do already).

If the approach you are opposed to is similar to what leptos provides for bubbling things up, I would like to understand your concerns a bit more. Especially I don't seem to see how abstracting things into a separate function to match on is a natural way for error handling (I'd say bubbling up custom error types until there is a component that can properly handle the error seems more rusty).

There is also the issue of allowing errors to bubble up completely, which could be achieved with a special context element for handling errors in components. Inside that, each component could have an entry, and then other components could check as necessary if something else has failed. This would be clearly useful for parents to check if their children have failed, while sibling checks would probably only be regarding whether or not some other part of context is accessible, in which case it should probably just be in a Result itself, external to any systems Sycamore might provide.

This is essentially what leptos does, yes (mapping the component by their hydration key to an error if any).

@lukechu10 lukechu10 removed this from the v0.9 milestone Nov 1, 2024
@davidon-top
Copy link
Contributor

davidon-top commented Nov 2, 2024

I think 2 error handling primitives should be added:

  1. Allow bubbling up errors easily
  2. Provide custom ErrorView type that can be turned .into() View
  3. (maybe) add way to transform a component output

In more details:

  1. could work by allowing ? inside of the view macro after components and after closures something like this
view! {
    ReturnsResult {}?
    (/*also returns a Result*/)?
}
  1. i would write something like:
struct ErrorView<E: Error, F: Fn(E) -> View> {
    inner: E,
    view: F
}
  1. (maybe) allow something like this where inside of () would be a closure that accepts the component return value
view! {
    ReturnsResult {}.(|it| it.unwrap())
}

I think this should be enough for most use cases. Also it goes hand in hand with classic rust error handling rather then the leptos approach mentioned above. This would makes the components feel like rust rather then js like.

I'm willing to make a pr for this

let me know if if this is good or you want a different design

@lukechu10
Copy link
Member

I have some doubts on having to manually bubble-up errors.

  1. Interaction with the children prop: It's not clear to me how we could make this work for component children. What I mean is since children right now is basically FnOnce() -> View and if we change it to FnOnce() -> ErrorView, we have to make a choice for all components that can take children. Either they must handle the error manually, or they must propagate it up to the caller. This would likely mean that all components that have children will need to be changed to ErrorView.
  2. Error handling is infectious: Related to the previous concern, if we want to handle errors at the top-level of our app and if we have a component that is very deeply nested that returns an error, we have to make the whole chain of components in between return ErrorView as well although their logic might not have any error producing code at all.

So although using the Leptos approach of introducing a ErrorBoundary component is less "Rusty", I think that ergonomics-wise, it will probably be better.

@lukechu10
Copy link
Member

Another requirement for fallible components should be good integration with resources and async APIs. I think this is probably the most likely scenario where one encounters errors, for instance, if the server returns something unexpected. So I think a context-based approach would work best here. I'm currently thinking of how we might want to do this.

@davidon-top
Copy link
Contributor

davidon-top commented Nov 3, 2024

if we change it to FnOnce() -> ErrorView

I should have been more specific with it i meant that a component would return `Result<View, ErrorView> and then it would be either bubbled or matched Err(e) => e.into() that way the parent component can decide if they want to handle the error in which case they would need the inner: E or just display it how the child component wants it

@davidon-top
Copy link
Contributor

davidon-top commented Nov 3, 2024

Personally i think that both top level and bubbling are very useful for different use cases so maybe both should be added but i'm unsure how they would coexist

@lukechu10
Copy link
Member

Personally i think that both top level and bubbling are very useful for different use cases so maybe both should be added but i'm unsure how they would coexist

I feel like explicitly bubbling errors should be done in data-fetching logic rather than in components. Looking at Leptos' approach, I think it's actually surprisingly clean: adding a Result<View> starts bubbling up an error until it finds an ErrorBoundary.

I think the problem with manually propagating errors is that a lot of the times, you don't get errors during creation of the component but after some interaction has happened. One thing that could be nice, however, would be to have a way to keep the concrete error type instead of getting it erased into a trait object. I'm not sure if that's possible though.

@davidon-top
Copy link
Contributor

problem with manually propagating errors is that a lot of the times, you don't get errors during creation of the component but after some interaction has happened

alright, u convinced me with this, I will take a look at this again after i'm done with query params in routes

@lukechu10
Copy link
Member

One thing that could be nice, however, would be to have a way to keep the concrete error type instead of getting it erased into a trait object. I'm not sure if that's possible though.

Actually this can be done just be matching on the signal like we would without error boundaries anyways so this is not too much of a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ergonomics Area: API ergonomics A-macro Area: macros C-enhancement Category: new feature or improvement to existing feature S-needs-design
Projects
None yet
Development

No branches or pull requests

4 participants