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

Add use_callback hook #2566

Merged
merged 2 commits into from
Apr 1, 2022
Merged

Add use_callback hook #2566

merged 2 commits into from
Apr 1, 2022

Conversation

jetli
Copy link
Contributor

@jetli jetli commented Apr 1, 2022

Description

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

@github-actions
Copy link

github-actions bot commented Apr 1, 2022

Visit the preview URL for this PR (updated for commit e92a224):

https://yew-rs--pr2566-use-callback-cm306hsq.web.app

(expires Fri, 08 Apr 2022 06:46:28 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Apr 1, 2022

Size Comparison

examples master (KB) pull request (KB) diff
boids 311.369 311.369 0
contexts 233.381 233.381 0
counter 164.549 164.549 0
dyn_create_destroy_apps 172.902 172.902 0
file_upload 194.978 194.978 0
function_memory_game 351.644 351.644 0
function_router 22.254 22.254 0
function_todomvc 327.006 327.006 0
futures 362.344 362.344 0
game_of_life 207.216 207.216 0
inner_html 156.415 156.415 0
js_callback 171.903 171.903 0
keyed_list 329.064 329.064 0
mount_point 163.721 163.721 0
nested_list 225.379 225.379 0
node_refs 170.802 170.802 0
password_strength 1851.783 1851.783 0
portals 184.307 184.307 0
router 589.899 589.899 0
suspense 222.801 222.801 0
timer 170.392 170.392 0
todomvc 270.743 270.743 0
two_apps 166.012 166.012 0
webgl 170.817 170.817 0

F: Fn(IN) -> OUT + 'static,
D: PartialEq + 'static,
{
let callback = use_state(|| -> RefCell<Option<Callback<IN, OUT>>> { RefCell::new(None) });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I am missing something, this can simply be
(*use_memo(move |_| Callback::from(f), deps)).clone()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, updated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then I'm wondering whether we need to add use_callback at all? but React uses useCallback a lot for performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another issue, the array of dependencies is not passed as arguments to the callback, if callback wants to consume deps, that might not be convenient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let oncallback = {
    let counter = counter.clone();
    let counter2 = counter.clone();
    use_callback(
        move |e| *counter2,
        counter
    )
};

vs

let oncallback = {
    let counter = counter.clone();
    use_memo(
        move |counter| {
            Callback::from(move |e| **counter)
        },
        counter
    )
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@futursolo any advice?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that React has a separate useCallback primarily due to useMemo(() => () => {}, []) looks a little bit complicated. I think in this case, if use_callback manages deps for you, it might still be worth it.

So one can do something like:

use_callback(move |deps| move |in_type| {}, deps);
// or have some magic by wrapping it in a custom closure.
use_callback(|deps, in_type| {}, deps);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use_callback(|deps, in_type| {}, deps); looks interesting, I'll have a try.

Copy link
Member

@voidpumpkin voidpumpkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use use_memo?
All i see this hook does is remove the boilerplate of writing Callback::from(/* my closure*/)

Copy link
Member

@voidpumpkin voidpumpkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah but i guess it wont hurt...

Love the docs and tests 👍

@voidpumpkin voidpumpkin merged commit 421b4e1 into yewstack:master Apr 1, 2022
@voidpumpkin voidpumpkin added the A-yew Area: The main yew crate label Apr 1, 2022
@ranile
Copy link
Member

ranile commented Apr 1, 2022

The only thing it changes is the reduced boilerplate and Rc::new to Rc::clone on creating a callback, right?

@jetli
Copy link
Contributor Author

jetli commented Apr 1, 2022

@hamza1311 use_memo returns Rc<T>, so Rc<Callback> will have a redundant Rc as Callback has already Rc internally. Instead use_callback returns Callback<IN, OUT> directly which is concise and can be used for DOM events directly.
The only thing now is whether we should use this signature use_callback(|deps, in_type| {}, deps); mentioned by @futursolo , which is convenient to consume deps, I'm still trying to figure it out.

@ranile
Copy link
Member

ranile commented Apr 1, 2022

I meant the difference between Callback::from and use_callback, not use_memo(|_| callback, ()) and use_callback. In that case, the only benefit I can think of is not recreating the Rc

@jetli
Copy link
Contributor Author

jetli commented Apr 2, 2022

The difference between Callback::from and use_callback is:

  • Callback uses Rc::ptr_eq for PartialEq, and Callback::from creates a new instance for every render, if parent comp passes callbacks to child comp, this may cause child comp to rerender every time.
  • use_callback uses use_memo internally, so it will return the same instance if last_deps == deps, then child comp will not rerender and gain some performance improvement.

React projects uses useCallback a lot, like const cb = useCallback(() => {}, []);

@jetli jetli deleted the use_callback branch April 2, 2022 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants