leptos-rs / leptos

Build fast web applications with Rust.
https://leptos.dev
MIT License
16.01k stars 626 forks source link

`Signal<T, S = SyncStorage>` doesn't implement `Track` #2976

Open maccesch opened 3 weeks ago

gbj commented 2 weeks ago

Note that it's not possible to implement Track as distinct from the behavior of Get for Signal, because derived signals can't be tracked without being called. Implementing Track here would cause (IMO) surprising behavior, because while calling .track() on a ReadSignal or Memo or other similar node simply tracks that node, calling .track() on a Signal potentially invokes that inner function, which may be relatively expensive.

A derived Signal that needs to be .track()ed is better represented as a Memo.

If it's just a question of API design and you want to be able to take #[prop(into)] Signal<_> or something and then track it, you can use with(|_| ()), which will not clone the values of a ReadSignal or Memo that is converted into a signal, but will invoke a derived signal. (Of course this is basically what a Track implementation would do, so maybe I'm overthinking it.)

Open to being wrong.

maccesch commented 2 weeks ago

Thanks for that comprehensive explanation. In fact I didn't think of it that deeply.

I ran into this when porting the new use_textarea_autosize in Leptos-Use to the new beta version. In the implementation there is one trigger_resize function that can be called manually and also automatically by several Effect::watches. And it's in these watches' dependency function where I just want to listen to if the signals are changed to call that function.

Now I indeed resorted to .with(|_| ()) but that feels kind of hacky. Then again, this obviously gives you more the feeling of "there's sth going on here" than just track() would. For this use case a Memo would actually be even less efficient because there's the overhead of it and the inner callback is called everytime, too. Just the trigger might not always be pulled it the result of the inner callback hasn't changed.

So I think there might be some valid use cases for having Singal.track(). Or what do you think?

zakstucke commented 1 week ago

Chiming in, not having track seems to prevent read()/write() being possible on signals, I'm finding these so ergonomic it would be unfortunate. It's also quite surprising as a user, I've no idea whether I have a root or derived signal half the time.

    let foo = RwSignal::new(0);
    let bar = Signal::derive(|| &*foo.read() + 1); // Success
    let baz = Signal::derive(|| &*bar.read() + 1); // doesn't satisfy `_: ReadUntracked`, `_: Read` or `_: Track`

ps. I understand this example would work if I just used get(), just simple!

gbj commented 1 week ago

prevent read()/write() being possible on signals

Again, this is because it isn't possible, and the trait system is trying to avoid lying to you, arguably at the expense of ergonomics. In fact, .read()/.write() will likely never be possible. .with() fakes it for derived signals by actually calling .get() and cloning and then applying the function to the reference, but for derived signals there is not underlying read or write lock being returned.

I want to distinguish really carefully between "possible on signals" and "possible on the Signal wrapper type." Signal is specifically designed to allow boxing up derived signals, in addition to ReadSignal/RwSignal/Memo (all of which do support .track(), .read(), and .write() in the case of RwSignal).

If you're in a situation where using .read() specifically is necessary and not just convenient, then you shouldn't accept Signal.

zakstucke commented 1 week ago

Whoops sorry definitely didn't mean to include write(), I was just hoping for read() on the Signal type. There's definitely no need, it's just for ergonomics.

Understood on the way Signal fakes .with() in 1/3 cases, but given it would be doing that anyway, couldn't we just... do the same thing for read by putting this cloned value in the read guard for the derived case? As far as I understand it this is an internal implementation detail / limitation the user doesn't need to know about anyway. I definitely may be missing something in which case sorry.

I think this logic might also apply to what @maccesch was porting over from 0.6, if it was using that "faking it" clone prior anyway.

Wouldn't something along the lines of this work, it seems to compile:

    /// 
    pub enum SignalReadGuard<T: 'static, S: Storage<T>> {
        ///
        Read(ReadGuard<T, Plain<T>>),
        ///
        Memo(ReadGuard<T, Mapped<Plain<MemoInner<T, S>>, T>>),
        ///
        Derived(T),
    }

    impl<T, S> Deref for SignalReadGuard<T, S>
    where
    T: 'static,
    S: Storage<T>
     {
        type Target = T;
        fn deref(&self) -> &Self::Target {
            match self {
                SignalReadGuard::Read(i) => i,
                SignalReadGuard::Memo(i) => i,
                SignalReadGuard::Derived(i) => i,
            }
        }
    }

    impl<T, S> ReadUntracked for ArcSignal<T, S>
    where
        S: Storage<T>,
        T: Clone,
    {
        type Value = SignalReadGuard<T, S>;

        fn try_read_untracked(&self) -> Option<Self::Value> {
            match &self.inner {
                SignalTypes::ReadSignal(i) => i.try_read_untracked().map(SignalReadGuard::Read),
                SignalTypes::Memo(i) => i.try_read_untracked().map(SignalReadGuard::Memo),
                SignalTypes::DerivedSignal(i) => {
                    Some(SignalReadGuard::Derived(untrack(|| i())))
                },
            }            
        }
    }

    impl<T, S> Read for ArcSignal<T, S>
    where
        S: Storage<T>,
        T: Clone,
    {
        type Value = SignalReadGuard<T, S>;

        fn try_read(&self) -> Option<Self::Value> {
            match &self.inner {
                SignalTypes::ReadSignal(i) => i.try_read().map(SignalReadGuard::Read),
                SignalTypes::Memo(i) => i.try_read().map(SignalReadGuard::Memo),
                SignalTypes::DerivedSignal(i) => {
                    Some(SignalReadGuard::Derived(i()))
                },
            }
        }

        fn read(&self) -> Self::Value {
            self.try_read().unwrap_or_else(unwrap_signal!(self))
        }
    }

I imagine it would be similar for the track trait, just no special guard.