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

Make equality works on noop callbacks of same type #3430

Closed
wants to merge 2 commits into from

Conversation

cecton
Copy link
Member

@cecton cecton commented Sep 30, 2023

Description

This make Callbacks NOOP equal to other noops of the exact same type.

Checklist

  • I have reviewed my own code
  • I have added tests

@github-actions
Copy link

github-actions bot commented Sep 30, 2023

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

https://yew-rs-api--pr3430-callback-eq-hlry4bwa.web.app

(expires Sat, 07 Oct 2023 09:05:48 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Sep 30, 2023

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 366.367 422.684 391.730 19.106
Hello World 10 590.404 705.993 643.723 40.090
Function Router 10 2103.914 2423.634 2223.732 100.976
Concurrent Task 10 1008.091 1010.382 1009.530 0.749
Many Providers 10 1418.927 1568.270 1508.135 56.420

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 375.312 430.253 399.060 22.779
Hello World 10 593.663 754.542 675.011 53.266
Function Router 10 2191.260 2534.028 2368.499 119.912
Concurrent Task 10 1007.654 1010.877 1009.217 0.927
Many Providers 10 1499.948 1719.462 1595.731 74.449

@github-actions
Copy link

github-actions bot commented Sep 30, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 102.914 102.914 0 0.000%
boids 175.670 175.670 0 0.000%
communication_child_to_parent 95.299 95.299 0 0.000%
communication_grandchild_with_grandparent 109.045 109.045 0 0.000%
communication_grandparent_to_grandchild 105.718 105.718 0 0.000%
communication_parent_to_child 92.793 92.793 0 0.000%
contexts 113.458 116.263 +2.805 +2.472%
counter 89.196 89.196 0 0.000%
counter_functional 89.934 89.934 0 0.000%
dyn_create_destroy_apps 92.319 92.319 0 0.000%
file_upload 103.514 103.514 0 0.000%
function_memory_game 174.635 174.635 0 0.000%
function_router 353.852 356.313 +2.462 +0.696%
function_todomvc 163.495 163.495 0 0.000%
futures 227.446 227.446 0 0.000%
game_of_life 112.218 112.218 0 0.000%
immutable 188.789 188.789 0 0.000%
inner_html 85.980 85.980 0 0.000%
js_callback 113.464 113.464 0 0.000%
keyed_list 201.211 201.211 0 0.000%
mount_point 89.188 89.188 0 0.000%
nested_list 114.613 114.613 0 0.000%
node_refs 96.287 96.287 0 0.000%
password_strength 1721.308 1721.308 0 0.000%
portals 98.366 98.366 0 0.000%
router 319.828 322.211 +2.383 +0.745%
simple_ssr 144.247 144.247 0 0.000%
ssr_router 391.655 394.117 +2.462 +0.629%
suspense 119.058 119.058 0 0.000%
timer 91.797 91.797 0 0.000%
timer_functional 100.498 100.498 0 0.000%
todomvc 143.686 143.686 0 0.000%
two_apps 89.897 89.897 0 0.000%
web_worker_fib 138.891 138.891 0 0.000%
web_worker_prime 190.393 190.393 0 0.000%
webgl 88.552 88.552 0 0.000%

⚠️ The following example has changed its size significantly:

examples master (KB) pull request (KB) diff (KB) diff (%)
contexts 113.458 116.263 +2.805 +2.472%

@cecton cecton marked this pull request as ready for review September 30, 2023 09:21
.unwrap()
.clone()
}),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there really no other solution other than unwrapping and modifying a HashMap in a RefCell? that seems to me like too much work for a Default::default() value

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the function is doing nothing I think it might be possible to do something with unsafe that would be much faster? But I'm really no expert so I won't write that lol

One alternative would be to wrap the Rc into an Option like this:

pub struct Callback<IN, OUT = ()> {
    /// A callback which can be called multiple times
    pub(crate) cb: Option<Rc<dyn Fn(IN) -> OUT>>,
}

(Or using an enum instead of struct)

Copy link
Member Author

Choose a reason for hiding this comment

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

But I'm afraid using Option will have a cost at runtime because every call will need to check the option.

@ranile
Copy link
Member

ranile commented Sep 30, 2023

What's the rationale behind this? No two closures are the same so why should two callbacks holding different closures be the same? Two Callbacks are the same if they have the same closure and that makes sense to me

@cecton
Copy link
Member Author

cecton commented Sep 30, 2023

@hamza1311 The case of the noop callback is very particular. Normally we should use Option<Callback> but sometimes you might want to create a component that forces the user to provide a callback. And if the user really wants to provide no callback at all, they would use noop.

Now the thing is that if they write this in their code:

<MyComponent
    onchange={Callback::noop()} />

The component will get updated all the time because the callback is re-created every time.

@cecton
Copy link
Member Author

cecton commented Oct 5, 2023

heh, I just bumped into #3395 and it clearly relates to my story

So maybe there is no need for making noop equal to other noops.

@cecton
Copy link
Member Author

cecton commented Oct 5, 2023

I'm closing this for now because I'm not sure. Anybody can re-open this ticket if they want.

@cecton cecton closed this Oct 5, 2023
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

Successfully merging this pull request may close these issues.

3 participants