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

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

Closed
maccesch opened this issue Sep 15, 2024 · 6 comments
Closed

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

maccesch opened this issue Sep 15, 2024 · 6 comments

Comments

@maccesch
Copy link
Contributor

No description provided.

@gbj
Copy link
Collaborator

gbj commented Sep 16, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

zakstucke commented Sep 26, 2024

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
Copy link
Collaborator

gbj commented Sep 26, 2024

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
Copy link
Contributor

zakstucke commented Sep 26, 2024

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.

@gbj
Copy link
Collaborator

gbj commented Oct 7, 2024

Added by #3031.

@gbj gbj closed this as completed Oct 7, 2024
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

No branches or pull requests

3 participants