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

Make Show component behave in a more intuitive way #2261

Closed
kcprs opened this issue Feb 5, 2024 · 5 comments
Closed

Make Show component behave in a more intuitive way #2261

kcprs opened this issue Feb 5, 2024 · 5 comments

Comments

@kcprs
Copy link

kcprs commented Feb 5, 2024

Is your feature request related to a problem? Please describe.

The Show component behaves in a way that is non-intuitive, especially to people who are new to fine-grained reactivity. For example, the call to unwrap() below seems safe because one would expect the children components to be rendered only if the when condition is true. In reality, the order in which effects react to the change of the selected_item signal is not deterministic, leading to spurious panics.

#[component]
fn SelectionView() -> impl IntoView {
    // struct SelectedItem(RwSignal<Option<usize>>);
    let selected_item = expect_context::<SelectedItem>().0.read_only();

    view! {
        <Show
            when=move || selected_item.get().is_some()
            fallback=|| {
                view! { "No item selected" }
            }
        >
            "Item "
            {move || selected_item.get().unwrap()}
            " is selected"
        </Show>
    }
}

Describe the solution you'd like

Perhaps it would be possible to ensure that effects run in the intuitive order.

@gbj
Copy link
Collaborator

gbj commented Feb 5, 2024

Thanks! Copying and pasting my reply from Discord so that it is visible here/I remember what I was talking about if I look at this in the future.

The explanation here is fairly simple, although it might not be satisfactory.

Because of the nature of fine grained reactive rendering, the Show component there creates two effects that both read from selected_item:

  1. Whether to show the fallback or children
  2. A text node that reads from selected_item

Updating selected_item triggers both of these, which depend on it. It is not necessarily the case that they run in the order you’re expecting, ie that the “show the fallback” effect runs first and cancels the “update a text node” effect by unmounting it.

.unwrap_or__default() is all you need here. If you want to delve into it further feel free to open a GitHub issue with the above and at some point I can explore whether we can guarantee that things run in the expected order and what that would entail

@gbj
Copy link
Collaborator

gbj commented Feb 5, 2024

Note: I cannot actually reproduce the issue you're describing with the example above.

Here's my attempt at a reproduction, which does not seem to panic at the .unwrap(), but correctly shows No item selected.

#[component]
pub fn App() -> impl IntoView {
    let (selected_item, set_selected_item) = create_signal(Some(1));

    view! {
        <button on:click=move |_| set_selected_item(None)>"None"</button>
        <button on:click=move |_| set_selected_item(Some(2))>"Some(_)"</button>
        <SelectionView selected_item />
    }
}

#[component]
fn SelectionView(selected_item: ReadSignal<Option<usize>>) -> impl IntoView {
    view! {
        <Show
            when=move || selected_item.get().is_some()
            fallback=|| {
                view! { "No item selected" }
            }
        >
            "Item "
            {move || selected_item.get().unwrap()}
            " is selected"
        </Show>
    }
}

@kcprs
Copy link
Author

kcprs commented Feb 5, 2024

Here is a repo with instructions on how to trigger the panic: https://github.com/kcprs/leptos-show-problem

@gbj
Copy link
Collaborator

gbj commented Feb 5, 2024

I'm happy to say: you win the "issue of the week" award, for turning up a very small but significant flaw in the implementation of the reactive system, and making life better for all users! 🎉

I've seen variations on this pop up occasionally for a while, but hadn't really looked at the root cause.

See comment added in #2262.

@gbj gbj closed this as completed in aec4d68 Feb 5, 2024
gbj added a commit that referenced this issue Feb 5, 2024
fix: guarantee execution order of effects (closes #2261)
@rakshith-ravi
Copy link
Contributor

We should have an awesome-issues like awesome-leptos lol

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

No branches or pull requests

3 participants