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

Implement signal.into_inner() #3343

Merged
merged 4 commits into from
Dec 17, 2024
Merged

Implement signal.into_inner() #3343

merged 4 commits into from
Dec 17, 2024

Conversation

stefnotch
Copy link
Contributor

This implements into_inner the way it is implemented for Arcs. This fixes #3068 .

However, with the non-arc signals, the behaviour can be rather unintuitive. For example, the following does not succeed.
Why?
Because there's still the original RwSignal that was created inside the signal function. (RwSignal::new(value).split() creates 3 signals)

#[test]
fn into_inner_non_arc_signal() {
    let owner = Owner::new();
    owner.set();

    let (a, b) = signal(2);
    assert_eq!(a.get(), 2);
    b.dispose(); // strong count is now 2
    assert_eq!(a.into_inner(), Some(2)); // fails
}

I'm happy to document this, or to attempt a fix, or to remove the trait for normal signals.

@gbj
Copy link
Collaborator

gbj commented Dec 17, 2024

I think my changes here to the implementation of signal() fix this, and cause the signal() test to work. It initially failed because signal() was implemented as RwSignal::new(value).split(), which means there's another reference to the inner ArcRwSignal (because it created an RwSignal and stored it in the owner). This version should be strictly better, in that it means only allocating 2 slots in the arena per signal() instead of 3, which is better for memory anyway. Thanks for surfacing that with the test, and for implementing this useful trait!

@gbj gbj merged commit 1661fe2 into leptos-rs:main Dec 17, 2024
73 of 74 checks passed
@stefnotch
Copy link
Contributor Author

Sweet, thank you for bringing this over the finish line.

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

Successfully merging this pull request may close these issues.

Signal::into_inner
2 participants