Prefer signals over memos with first unused argument when possible #76
Conversation
mondeja
changed the title
mondeja
force-pushed
the
signals-over-memos
branch
from
477f624 to
16e67c2
Compare
|
Hey thanks for this. I don't understand the motivation behind this change though. What's the reasoning here? |
|
See the documentation about
|
|
😄 I know about this documentation. But if you read carefully you see that there are cases where memos are preferred. I think you're definitely correct about changing to derived signals in the sorting functions I'm not so sure about the other ones. While thinking about it I'm actually considering changing other "from" conversions to memos as well because usually the stuff that depends on these reactive elements is rather expensive. Although I'm not sure how that compares to the memo overhead. What do you think? |
| @@ -208,8 +208,7 @@ macro_rules! impl_from_signal_string { | |||
| Self::Dynamic(Signal::derive(|| None)) | |||
| } else { | |||
| Self::Dynamic( | |||
| create_memo(move |_| document().query_selector(&signal.get()).unwrap_or_default()) | |||
| .into(), | |||
| Signal::derive(move || document().query_selector(&signal.get()).unwrap_or_default()), | |||
There was a problem hiding this comment.
Perhaps here only Memo<...>s conversions should use create_memo() to maintain the same behaviour in conversions? I was thinking that as signal.get() is used here inside the memo/signal, it would react as a memo only if the underlying signal is a memo.
There was a problem hiding this comment.
I understand what you mean, though in Memo conversions it would be unnecessary really, because it's already memoized. In that case it would actually only create overhead.
But in the other cases... I'm not sure. We could make the argument that the code that uses these (which is a lot of functions in leptos-use) should create their own memo if they think they need it.
I'm torn...
There was a problem hiding this comment.
We could make the argument that the code that uses these (which is a lot of functions in leptos-use) should create their own memo if they think they need it.
Note that the current behaviour could create a lot of headaches when trying to debug why a signal is not being retriggered.
There was a problem hiding this comment.
You're right! Sorry about flipflopping here, but I think you were right with your first PR.
There was a problem hiding this comment.
No problem 👍🏼 Reverted to the original state.
| @@ -86,7 +86,7 @@ where | |||
| let index = { | |||
| let list = list.clone(); | |||
|
|
|||
| create_memo(move |_| { | |||
| Signal::derive(move || { | |||
There was a problem hiding this comment.
Seems to me that this should be a derived signal because is supposed to always throw a new value.
mondeja
force-pushed
the
signals-over-memos
branch
from
9ea0170 to
d366909
Compare
Use
impl Fn() -> T + 'staticinstead ofimpl Fn(Option<&T>) -> T + 'staticwhen possible.