Resupport From<Fn() -> T> for Signal<T>, ArcSignal<T>, Callback<T, _> and similar

[zakstucke]

Rough draft to enable support for reactive closures to auto convert to Signal, without having to remove the From<T> implementation.

Requires a tweak to typed-builder here

We can break the "conflicting implementations" issue with a marker (M) struct on an IntoSignal<T, M> trait.

fn my_interface(_sig: Signal<usize>) {}

my_interface(2.into_signal())
my_interface((|| 2).into_signal()) // This will use a different M

which can instead be written as:

fn my_interface<M>(val: impl IntoSignal<Signal<usize>, M>) {
    let _sig = val.into_signal();
}

This means we need an <M> generic on the builder method generated by typed-builder, which already supports calling a custom "transformer", just not with generics, hence the new transform_generics config I added here.

This in leptos_macro becomes in the case of Signal<usize>:

#[derive(TypedBuilder)]
struct Foo {
    #[builder(setter(
                     transform_generics = "<M>", 
                     transform = |value: impl IntoSignal<Signal<usize>, M>| value.into_signal()
                )
    )]
    sig: Signal<usize>,
}

This works!!

#[component]
pub fn Outer() -> impl IntoView {
    let foo = move || "I was a reactive closure!";
    let bar = "I was a basic str!";
    let baz = Signal::stored("I was already a signal!");
    let loo = move || 2;
    let lah = 3;
    let liz = Signal::stored(4);

    view! { <Inner foo=foo bar=bar baz=baz loo=loo lah=lah liz=liz /> }
}

#[component]
pub fn Inner(
    #[prop(into)] foo: Signal<String>,
    #[prop(into)] bar: Signal<String>,
    #[prop(into)] baz: Signal<String>,
    #[prop(into)] loo: Signal<usize>,
    #[prop(into)] lah: Signal<usize>,
    #[prop(into)] liz: Signal<usize>,
) -> impl IntoView {
    move || {
        view! {
            <div>
                <p>{foo.get()}</p>
                <p>{bar.get()}</p>
                <p>{baz.get()}</p>
                <p>{loo.get()}</p>
                <p>{lah.get()}</p>
                <p>{liz.get()}</p>
            </div>
        }
    }
}

This can be extended to support other types, ReadSignal, Callback etc where needed too

[benwis]

So my first reaction was Woah and my second was How? so you've got a pretty interesting PR there. I'd love to not have to manually derive signals. Does the addition of generics on the generated into functions affect compile times at all?

[zakstucke]

Does the addition of generics on the generated into functions affect compile times at all?

I haven't tested, I would assume non-zero, but also that it wouldn't be meaningful, I'm not actually asking the compiler for much here, it's an A/B generic, so there's not any type explosion or anything.

[benwis]

Does the addition of generics on the generated into functions affect compile times at all?

I haven't tested, I would assume non-zero, but also that it wouldn't be meaningful, I'm not actually asking the compiler for much here, it's an A/B generic, so there's not any type explosion or anything.

I expect it to be minimal as well, just curious.

[zakstucke]

I've made it robust and not specific to signals, I've added support for Callback<T, _> too, where before you'd be forced to do Callback<(T,), _> and use (|(_,)| {}).into(), now Callback<T, _> and (|_| {}).into() works too.

This required me to move the callback module into reactive_graph though, the method only works with types defined inside the same crate.

[zakstucke]

The semver CI failing is a false positive I believe, it doesn't understand the old callback module is still available, it's just sourced from reactive_graph now instead. The paths it complains are missing are all fine, leptos::callback::*, leptos::prelude::Callback etc.

I'm happy with this PR now I think, it's just blocked by the upstream PR:
idanarye/rust-typed-builder#169

[gbj]

... Wow! 😍 This is really great. Honestly kind of mindblowing.

I understand why using a marker generic lets us get around the conflicting trait implementations (they are no longer conflicting.)

I'm a little worried about the potential for this to break in subtle ways where the compiler can't infer the type of the marker. (I haven't thought it through that fully, or found cases where it does break -- I'm just used to the compiler enough to anticipate it! 😄)

My only comment other than the individual nits is a bikeshedding one about the name: if we're putting it in the non-leptos-specific reactive_graph trait perhaps IntoReactiveValue or something rather than IntoLeptosValue?

In any case, awesome work and an ingenious solution.

[zakstucke]

I'm a little worried about the potential for this to break in subtle ways where the compiler can't infer the type of the marker. (I haven't thought it through that fully, or found cases where it does break -- I'm just used to the compiler enough to anticipate it! 😄)

😅 I haven't been able to find a valid case it fails yet, as long as the T in Signal<T> is concretely known, the compiler seems to have no issue with it, which was already a requirement with our signal implementations. The fact it can handle || &'static str -> Signal<String> without issues (the specific conversion is implemented mind), gives me more confidence.

I'd encourage you to try and break it though!

My only comment other than the individual nits is a bikeshedding one about the name: if we're putting it in the non-leptos-specific reactive_graph trait perhaps IntoReactiveValue or something rather than IntoLeptosValue?

Very happy to change it, but I don't think IntoReactiveValue works for Callback. It's also something a user might want to use directly, if they want a function that takes foo<M>(val: impl IntoLeptosValue<Signal<usize>, M>) or use value.into_leptos_value(). Also, this IntoLeptosValue trait is being used instead of .into() for all #[prop(into)], we might want to have implementations on all sorts of types down the line.

[benwis]

Not sure I like IntoLeptosValue either, not really clear what a LeptosValue is 😛

[maccesch]

I'm a little worried about the potential for this to break in subtle ways where the compiler can't infer the type of the marker. (I haven't thought it through that fully, or found cases where it does break -- I'm just used to the compiler enough to anticipate it! 😄)

We're actually using the same kind of trick with Effect since 0.7 that accepts a function with and without parameters. And I haven't come across a case yet where the compiler couldn't infer the marker type. I suspect it's going to be same here.

[gbj]

Thanks @maccesch. There were a few places I came across inference issues with the Effect changes, in places where it had trouble inferring the type of T in prev: Option<T> and I had to manually specify the type Effect::new(move |prev: Option<()>| or something similar.

For example:

#[component]
fn App() -> impl IntoView {
    Effect::new(move |prev| {
        if prev.is_some() {
            log!("not the first run");
        }
        log!("foo bar {prev:?}");
    });
}

compiled on 0.6, but on 0.7/0.8 it yields the error

error[E0282]: type annotations needed
 --> src/main.rs:6:23
  |
6 |     Effect::new(move |prev| {
  |                       ^^^^
7 |         if prev.is_some() {
  |            ---- type must be known at this point
  |
help: consider giving this closure parameter an explicit type
  |
6 |     Effect::new(move |prev: /* Type */| {

(The changes to Effect are still a net win, in my opinion, to be clear!)

It's possible that this kind of inference error doesn't apply here because the component prop type needs to be provided. I haven't fully though through how the two implementations parallel each other... (and I'm going to log off for the evening instead!)

[maccesch]

Understood. Excited to see where you land on this.

[zakstucke]

Should be good to go now other than any bikeshedding on IntoLeptosValue.

As mentioned before the failed semver CI check is a false positive.

[sabify]

I think the failed semver check is due to moving callback module from leptos to reactive_gragh and it shouldn't be a false positive...

[zakstucke]

I think the failed semver check is due to moving callback module from leptos to reactive_gragh and it shouldn't be a false positive...

The callback module is then re-exported from the leptos crate, and is accessible identically as before, cargo-semver-checks just doesn't seem to realise that. Unless you see something I'm missing?

[gbj]

I guess I'm okay with just disabling this on nightly, although it raises a moderately large issue: namely that it would prevent us from stabilizing the function-call syntax once the relevant nightly traits are stabilized without removing this implementation again (which would be a breaking change).

I may be having trouble remembering the history here.

Was the situation in 0.6 that Signal<T> supported From<Fn() -> T> but it did not implement Fn() on nightly? That seems plausible. In which case maybe removing the Fn() implementation for Signal<_> in 0.9 would allow us to solve this problem by unifying the two things.

In any case I am sure that merging this is the right idea -- I will just wait for the CI to finish running.

[gbj]

Oh and re: bikeshedding — are you okay with IntoReactiveValue/.into_reactive_value()?

[zakstucke]

Oh and re: bikeshedding — are you okay with IntoReactiveValue/.into_reactive_value()?

I'm okay with anything, although I'd hesitate that IntoReactiveValue doesn't make sense with things like Callback, which are already using and benefit from it. If it makes sense to you though I have no objection, for the main intended use case it's completely behind the scenes anyway.

I may be having trouble remembering the history here.
Was the situation in 0.6 that Signal supported From<Fn() -> T> but it did not implement Fn() on nightly? That seems plausible.

It's a good question, I'm unsure too and could check deeper next week but a quick check back in the git history seems like it might have been a 0.8 feature (61f5294, 1f2b13a)? Either way I think the standardisation to this PR in a 0.9 for nightly also, rather than keeping the nightly Fn() feature which doesn't have a guarantee of stabilising, does make sense. Worth mentioning this PR still improves the Callback type on nightly today, the Fn() feature only blocks the signal improvements specifically, not the overall PR.

[zakstucke]

Was the situation in 0.6 that Signal supported From<Fn() -> T> but it did not implement Fn() on nightly?

I checked this out, and it did support both by utilising auto_traits and negative_impls! I've been able to use the same trick here, so it actually now works on nightly too 🎉