-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Allow to consume deps in use_callback hook #2617
Conversation
Visit the preview URL for this PR (updated for commit 7e9951d): https://yew-rs--pr2617-refactor-use-callbac-8kuzgr97.web.app (expires Fri, 22 Apr 2022 16:14:19 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
@futursolo We talked about this in my previous PR, now I'm changing the signature of |
Size Comparison
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the signature is a bit more complicated than before,
I think in practice, a dependency for use_callback
is almost always desirable.
So, this looks good to me.
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for implementing it
D: PartialEq + 'static, | ||
{ | ||
(*use_memo(move |_| Callback::from(f), deps)).clone() | ||
let deps = Rc::new(deps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use D: PartialEq + 'static + Clone
to avoid this allocation, since in most cases the deps have already implemented Clone
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Rc Clone is usually more cheap than Clone of deps, as Rc is just like a pointer, and we don't want to constrain Clone to end user. and let's be consistent with use_state_eq/use_effect_with_deps etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Not really, since this hook is used in the case that deps is not changed frequently, assume the ratio is 100:1. when this hook is called 100 times, using Rc needs 100 alloc + 100 dealloc, but using Clone just needs 1
deps.clone
since the Callback is memoized. - For consistence, the
use_context
hook also usesClone
, so this is not a special one. - All the
deps
params ofuse_effect_with_deps
in this repo implement Clone, so users are not constrained actually. - If in some rare cases users provide a not Clone deps, they can call Rc::new at the calling site.
Description
This pr refactors
use_callback
hook to allows to consume deps, e.g.This is a break change of previous design, the signature of
use_callback
changes fromuse_callback(|in_type| {}, deps);
touse_callback(|in_type, deps| {}, deps);
.Checklist
cargo make pr-flow