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

Diffing cloned Components with readonlysignal panics on double drop #2331

Open
3 tasks
ealmloff opened this issue Apr 17, 2024 · 1 comment · May be fixed by #2839
Open
3 tasks

Diffing cloned Components with readonlysignal panics on double drop #2331

ealmloff opened this issue Apr 17, 2024 · 1 comment · May be fixed by #2839
Assignees
Labels
bug Something isn't working core relating to the core implementation of the virtualdom
Milestone

Comments

@ealmloff
Copy link
Member

Problem

Any props with the T -> ReadOnlySignal logic fail to diff if you try to render it multiple times:

This is the signal version of the EventHandler problem fixed in #2298

Steps To Reproduce

Run this code:

use dioxus::prelude::*;

fn main() {
    launch(app);
}

fn app() -> Element {
    if generation() < 5 {
        println!("Generating new props");
        needs_update();
    }

    let read_only_signal_rsx = rsx! {
        // # Creation
        // This creates a new read only signal which is owned by the props
        // There is currently no Clone requirement to use this syntax
        // 
        // # Diffing
        // When memorizing the props, if the value is different then value is taken out of the signal
        TakesReadOnlySignal { sig: generation() as i32 }
    };

    rsx! {
        // When this is diffed, the value is moved out of read_only_signal_rsx
        {read_only_signal_rsx.clone()}
        // When this is diffed, it panics because the value has already been moved out of the signal in the previous line
        {read_only_signal_rsx}
    }
}

#[component]
fn TakesReadOnlySignal(sig: ReadOnlySignal<i32>) -> Element {
    rsx! {}
}

Expected behavior

A clear and concise description of what you expected to happen.

Screenshots

If applicable, add screenshots to help explain your problem.

Environment:

  • Dioxus version: master
  • Rust version: nightly
  • OS info: MacOS
  • App platform: all

Questionnaire

  • I'm interested in fixing this myself but don't know where to start
  • I would like to fix and I have a solution
  • I don't have time to fix this right now, but maybe later
@ealmloff ealmloff added bug Something isn't working core relating to the core implementation of the virtualdom labels Apr 17, 2024
@ealmloff
Copy link
Member Author

I can think of two different ways to fix this issue:

  1. Introduce an extra layer of interior mutability in every ReadOnlySignal that allows you to update the signal it points to in place. Instead of moving the readonlysignal out of the props, we could just make the readonlysignal in the component point to the new signal. This is non-breaking, but it introduces a performance penalty for all readonlysignals
  2. Introduce a difference between the mounted and unmounted props. The unmounted props for TakesReadOnlySignal { sig: generation() as i32 } could be the i32 value which is cloned along with the component. The ReadOnlySignal could then be created in the child component instead of moved out of the props

@ealmloff ealmloff self-assigned this Jul 22, 2024
@jkelleyrtp jkelleyrtp added this to the 0.6.0 milestone Jul 25, 2024
@ealmloff ealmloff linked a pull request Aug 14, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core relating to the core implementation of the virtualdom
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants