DioxusLabs / dioxus

Fullstack GUI library for web, desktop, mobile, and more.
https://dioxuslabs.com
Apache License 2.0
18.38k stars 699 forks source link

Element components cannot be reused #1032

Closed Niedzwiedzw closed 2 months ago

Niedzwiedzw commented 11 months ago

Problem

use super::*;

#[derive(Props)]
pub struct DropdownMenuProps<'children> {
    dropdown_content: Element<'children>,
    controller_open: Element<'children>,
    controller_closed: Element<'children>,
}

pub fn DropdownMenu<'children>(cx: Scope<'children, DropdownMenuProps<'children>>) -> Element {
    let open = use_state(cx, || false);
    let onclick = |_| open.set(!open.get());

    cx.render(rsx!(
        div { class: "DropdownMenu",
            div { class: "controller-content", onclick: onclick,
                if *open.get() {
                    &cx.props.controller_open
                } else {
                    &cx.props.controller_closed
                }
            }
            div { class: "dropdown-content",
                open.get().then_some(()).and_then(|_| cx.props.dropdown_content.as_ref())
            }
        }
    ))
}
panicked at 'called `Option::unwrap()` on a `None` value', /home/niedzwiedz/.cargo/git/checkouts/dioxus-1e619ce595d3799d/6acec6f/packages/core/src/diff.rs:213:57

above component crashes after I toggle it 3 times.

Steps To Reproduce dioxus = { git = "https://github.com/DioxusLabs/dioxus", rev = "c97f43146d17abca049f77a616ae6b116d90ca34" } dioxus-web = { git = "https://github.com/DioxusLabs/dioxus", rev = "c97f43146d17abca049f77a616ae6b116d90ca34" }

Niedzwiedzw commented 11 months ago

discord thread: https://discord.com/channels/899851952891002890/899851952891002893/1108757735522177147

ealmloff commented 11 months ago

Could you post the props you pass into the element in the example where it crashes?

I tested this code and could not reproduce the issue:

use dioxus::prelude::*;

fn main() {
    dioxus_web::launch(app);
}

fn app(cx: Scope) -> Element {
    let dynamic = String::from("testing");
    cx.render(rsx!(DropdownMenu {
        dropdown_content: render! {
            div { "Hello, world! {dynamic}" }
        },
        controller_open: render! {
            div { "Close {dynamic}" }
        },
        controller_closed: render! {
            div { "Open {dynamic}" }
        },
    }))
}

#[derive(Props)]
pub struct DropdownMenuProps<'children> {
    dropdown_content: Element<'children>,
    controller_open: Element<'children>,
    controller_closed: Element<'children>,
}

pub fn DropdownMenu<'children>(cx: Scope<'children, DropdownMenuProps<'children>>) -> Element {
    let open = use_state(cx, || false);
    let onclick = |_| open.set(!open.get());

    cx.render(rsx!(
        div { class: "DropdownMenu",
            div { class: "controller-content", onclick: onclick,
                if *open.get() {
                    &cx.props.controller_open
                } else {
                    &cx.props.controller_closed
                }
            }
            div { class: "dropdown-content",
                open.get().then(|| cx.props.dropdown_content.as_ref())
            }
        }
    ))
}
Niedzwiedzw commented 11 months ago

https://github.com/Niedzwiedzw/dioxus-crash-borrow-mut

I created a repo that showcases the exact scenario... I needed to recreate it cause the original contains some SECRET stuff :D but this is the same scenario and it's somewhat minimal @Demonthos

ealmloff commented 11 months ago

I have a guess what is happening here:

1) When components are diffed the prop of the new component is taken 2) If a component is reused between renders, the component has no props the second time it is used which causes the panic

Niedzwiedzw commented 11 months ago

hmm so is it a bug or am I doing something against the rules?

ealmloff commented 11 months ago

hmm so is it a bug or am I doing something against the rules?

This is an unintended consequence of the rule that you cannot use an element twice in rsx (in your example they end up getting used twice: Once in the first render, and once in the second render). I would consider it a bug.

One possible solution is to wrap the props in Rc to keep the reference alive longer so that if you reuse a prop multiple times, the reference will stay alive until the last use is dropped.

Niedzwiedzw commented 11 months ago

hmmm but am I not returning a closure which creates a new element every time?

Niedzwiedzw commented 11 months ago

by the way, if you scroll down, there is another panic - something about borrowing some value while it is already borrowed. not sure which one causes the other, but the error didn't occur without using shared state right?

ealmloff commented 11 months ago

hmmm but am I not returning a closure which creates a new element every time?

You do recreate the element in the parent component (CounterDropdown), but when the child component (DropdownMenu) reruns by itself because of the open hook updating, the elements in the props are the same as the elements used in the initial render with a component that has had it's props .taken

by the way, if you scroll down, there is another panic - something about borrowing some value while it is already borrowed. not sure which one causes the other, but the error didn't occur without using shared state right?

That is pretty normal with rust/WASM panics. It is the same bug

Niedzwiedzw commented 11 months ago

@Demonthos I've pushed an update, this is the minimal change that fixes it (you need to uncomment the "fix" line to apply the "fix")

Niedzwiedzw commented 11 months ago
use std::{
    collections::hash_map::DefaultHasher,
    hash::{Hash, Hasher},
};
​
use dioxus::prelude::*;
use tracing::debug;
​
fn calculate_hash<T: Hash>(t: &T) -> u64 {
    let mut s = DefaultHasher::new();
    t.hash(&mut s);
    s.finish()
}
​
/// dioxus has a limitation:  components cannot be reused... this works around that
pub fn frag<'scope, Props, Builder, ScopeProps>(
    cx: Scope<'scope, ScopeProps>,
    props: Props,
    builder: Builder,
) -> Element<'scope>
where
    Props: Hash,
    Builder: Fn(Props) -> LazyNodes<'scope, 'scope>,
{
    let hash = calculate_hash(&props);
    debug!(
        "[{}] -> [{}]: fragment is redrawing",
        std::any::type_name::<ScopeProps>(),
        std::any::type_name::<Props>()
    );
​
    cx.render(rsx! {
        Fragment {
            key: "{hash}",
            builder(props)
        }
    })
}

so I wrote this as a workaround, could someone please double check me? are there any downsides to doing it this way?