DioxusLabs / dioxus

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

MappedSignal example crashes on pop. General MappedSignal weirdness #2059

Closed agreyyy closed 4 months ago

agreyyy commented 4 months ago

Today I was playing around with MappedSignals via the map_signal.rs example in the signals examples folder. This example worked fine, except for the fact that it would crash whenever I clicked the "Detroy" button/popped a value off the vec.

button {
    onclick: move |_| {
        vec.write().pop();
    },
    "Destroy"
}

heres the logs from the example:

thread 'main' panicked at src/main.rs:30:47:
index out of bounds: the len is 0 but the index is 0

after launching the app and simply clicking the destroy button

line 30 is the for loop which loops over each index in the vec and maps it to a MappedSignal. For some reason the len of the vec shrinks by one (the pop operation) after the for loop is initialized, causing it to crash when the vec.map() function accesses the last index provided to it by the loop. This was fixed by just clamping the index inside the vec.map() closure like so:

vec.map(move |v| &v[i.clamp(0, v.len().checked_sub(1).unwrap_or(0))])

but this is ugly and still fails if the vec is empty, so I think its fair to say that MappedSignals are breaking something in this code. Another thing that I noticed was that the memos inside the child components re ran each time, not depending on whether the value of the MappedSignal changed or not, This probably means that MappedSignal are the broken ones, since memos have no trouble only re running when other signals change, but I think this could be fixed by the #2029 PR since these are child memos, which are a bit buggy sometimes, but the weirdest thing was that the memos ran out of order (see logs), which I thought they wouldn`t since they are synchronous??

Steps To Reproduce

  1. Just run the map_signal.rs example

Expected behavior

  1. the child components` memos only get re ran when the value of the index of the vector changes (new element is pushed, in this context)
  2. the app doesnt crash when we pop a value off the vector
  3. the child components dont re render at all if the vector index they are mapped to does not change which they currently do, the printlns are the same if you put them outside the memos) changing the child component to this

    #[component]
    fn Child(count: MappedSignal<i32>) -> Element {
    println!("Child value: {count}");
    
    rsx! {
        div {
            "Child: {count}"
        }
    }
    }

    produces basically the same logs, but even weirder:

    //app starts
    Child value: 0
    //clicked create 1st time
    Child value: 1
    Child value: 0
    //clicked create 2nd time
    Child value: 2
    Child value: 0
    Child value: 1
    //clicked create 3 time??
    Child value: 3
    Child value: 0
    Child value: 1
    Child value: 2 

    because now the elements print their logs out of order starting from the 2nd log, and with the same pattern (last element in front, then the rest) which keeps going after pressing create more than 3 times as well. This is different from the memos because they had no pattern at all. Anyway Mapped Signals are a really cool idea and I hope that you guys can get them to work someday, thank you for all your work for 0.5 already, it is a major improvement over 0.4.

Environment:

ealmloff commented 4 months ago

This is an issue with memos. The memo is being run after the child component should be dropped which causes the index out of bounds issue. The mapped signal example works in #2029

ealmloff commented 4 months ago
  1. the child components` memos only get re ran when the value of the index of the vector changes (new element is pushed, in this context)
  2. the child components dont re render at all if the vector index they are mapped to does not change which they currently do, the printlns are the same if you put them outside the memos) changing the child component to this

The current behavior is intentional. MappedSignal just maps reads from a signal. It doesn't clone any data, or memorize values. This makes it possible to use MappedSignal with data that is not Clone or PartialEq. If you would like to memorize the result, you can use the use_memo hook. Because it doesn't memorize the result, MappedSignal has the same subscriptions as the orignal signal.

For Vecs, HashMaps, and HashSets, it would be nice to provide a more efficient Signal version with smarter element subscriptions.