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

Remove From<T> and add back From<Fn() -> T> #3386

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mscofield0
Copy link
Contributor

The reason for is that the new From impls focus more on making MaybeSignal work for Signal since now it is removed, but the consequence of it is that:

#[component]
fn FooComponent(
    #[prop(into)]
    is_true: Signal<bool>,
) -> impl IntoView { ... }

<FooComponent is_true=move || blabla.get().is_true />

doesn't work. Ideally, Signal should have both Froms, but I don't see how that's possible, so I believe that supporting the common case (the non-MaybeSignal one) should be preferred.

WARNING

This commit is a breaking change because it removes and adds a couple of trait impls.

@mscofield0 mscofield0 changed the title Remove From<T> and add back From<Fn() -> T> Remove From<T> and add back From<Fn() -> T> Dec 18, 2024
@mscofield0
Copy link
Contributor Author

Weird CI errors, compiles both on stable and nightly locally for me...

@gbj
Copy link
Collaborator

gbj commented Dec 19, 2024

Thanks for the PR!

A couple notes:

  1. nightly errors

Weird CI errors, compiles both on stable and nightly locally for me...

No, it doesn't compile with the nightly feature. Using the nightly compiler does not activate the nightly features; try cd reactive_graph && cargo check --features=nightly (when using the nightly toolchain) and you will see the compiler error. The problem is that simply implementing From<F: Fn() -> T> causing another set of conflicting implementations, because From<ReadSignal<T>> etc. are still implemented, but with the nightly feature they also implement Fn() themselves.

The 0.6 implementation relied on the auto_trait/negative_impls unstable nightly features to work around this; you can consult the leptos_0.6 branch if you want to see how that worked.

  1. We do want to support both T and Fn() -> T but unfortunately that's not possible. This version does support both, for FooComponent -- it would be something like <FooComponent is_true=false/> and <FooComponent is_true=Signal::derive(move || ...)/>. In retrospect, perhaps the better choice would've been to support <FooComponent is_true=Signal::stored(false)/> and <FooComponent is_true=move || .../> as that removes the breaking change here. This simply wasn't caught by anyone who was testing during the alpha/beta/rc period over the last 6 months, unfortunately.

Note that this change doesn't make this less expressive, it just adds a bit of boilerplate in the derived signal case.

I'm open to changing back to that second option in the next version, although unfortunately that will just be another breaking change at that point. It's not clear to me whether the reactive (move || blabla.get().is_true) or static (foo) option should be the default, but probably the reactive one I guess. However,

  1. Unfortunately this can't be merged at the moment anyway because of the breaking change, but I will mark it with the 0.8 milestone to remind myself to come back to this conversation when we're going to have another major release. (I anticipated some things like this coming up and I'm planning on releasing a 0.8 with minor breaking changes like this in late January/early Feb)

@gbj gbj added the breaking label Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants