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 CallbackRef that takes ref in argument instead of value #3419

Merged
merged 8 commits into from
Sep 29, 2023

Conversation

cecton
Copy link
Member

@cecton cecton commented Sep 27, 2023

Description

It's sometimes useful to pass callback functions to components. The struct
Callback can be used for that but it only accept passing the input argument by
value. It is useful sometimes to pass the input argument by reference instead.

Alternative explanation from @kirillsemyonkin :

Its for doing Fn(&IN) -> OUT instead of Fn(IN) -> OUT since some rust weirdness.

Doing Callback<&IN, OUT> says that the type should have a 'a which is wrong because its Fn under the hood.

It would've worked if we were able to write Callback<&IN> -> OUT or whatever but I think Rust allows that syntax only for Fn family.

Checklist

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

@github-actions
Copy link

github-actions bot commented Sep 27, 2023

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

https://yew-rs-api--pr3419-callback-ref-eocw38vx.web.app

(expires Wed, 04 Oct 2023 12:51:40 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Sep 27, 2023

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 319.503 320.053 319.703 0.173
Hello World 10 652.113 668.791 662.738 5.234
Function Router 10 2154.012 2165.300 2159.260 3.924
Concurrent Task 10 1007.025 1008.103 1007.671 0.331
Many Providers 10 1506.672 1533.849 1516.875 8.301

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 319.589 319.797 319.696 0.061
Hello World 10 658.760 673.624 668.083 4.246
Function Router 10 2153.638 2169.620 2160.019 5.312
Concurrent Task 10 1006.946 1008.382 1007.784 0.510
Many Providers 10 1529.510 1557.193 1537.271 7.839

@github-actions
Copy link

github-actions bot commented Sep 27, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 102.960 102.960 0 0.000%
boids 175.618 175.618 0 0.000%
communication_child_to_parent 95.298 95.298 0 0.000%
communication_grandchild_with_grandparent 109.034 109.034 0 0.000%
communication_grandparent_to_grandchild 105.710 105.710 0 0.000%
communication_parent_to_child 92.792 92.792 0 0.000%
contexts 113.453 113.453 0 0.000%
counter 89.196 89.196 0 0.000%
counter_functional 89.936 89.936 0 0.000%
dyn_create_destroy_apps 92.369 92.369 0 0.000%
file_upload 103.513 103.513 0 0.000%
function_memory_game 174.570 174.570 0 0.000%
function_router 353.673 353.673 0 0.000%
function_todomvc 163.505 163.505 0 0.000%
futures 227.445 227.445 0 0.000%
game_of_life 112.163 112.163 0 0.000%
immutable 188.785 188.782 -0.003 -0.002%
inner_html 85.979 85.979 0 0.000%
js_callback 113.458 113.458 0 0.000%
keyed_list 201.203 201.203 0 0.000%
mount_point 89.189 89.189 0 0.000%
nested_list 114.618 114.618 0 0.000%
node_refs 96.290 96.290 0 0.000%
password_strength 1721.062 1721.062 0 0.000%
portals 98.365 98.365 0 0.000%
router 319.654 319.654 0 0.000%
simple_ssr 144.237 144.237 0 0.000%
ssr_router 391.450 391.450 0 0.000%
suspense 119.067 119.067 0 0.000%
timer 91.745 91.745 0 0.000%
timer_functional 100.451 100.451 0 0.000%
todomvc 143.688 143.688 0 0.000%
two_apps 89.897 89.897 0 0.000%
web_worker_fib 138.871 138.871 0 0.000%
web_worker_prime 190.426 190.426 0 0.000%
webgl 88.554 88.554 0 0.000%

✅ None of the examples has changed their size significantly.

@cecton cecton marked this pull request as ready for review September 27, 2023 13:02
Comment on lines +49 to +55
impl<IN> $callback<IN> {
/// Creates a "no-op" callback which can be used when it is not suitable to use an
/// `Option<$callback>`.
pub fn noop() -> Self {
Self::from(|_: $in_ty| ())
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this is actually wrong because creating 2 "noop" callbacks and comparing them for equality will yield false

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine as it is trying to compare the address of 2 functions are not the content of the functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I should have written a test -_-

The PartialEq implementation uses Rc::ptr_eq() so it's actually comparing that it is the same RC but we create one each time we call noop (or Default).

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3e6510283a5655f1166410d77fd62d51

Copy link
Member Author

Choose a reason for hiding this comment

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

I made this PR to address this: #3430

@cecton cecton mentioned this pull request Sep 27, 2023
2 tasks
@ranile
Copy link
Member

ranile commented Sep 27, 2023

What prevents you from doing Callback<&I, O>

@cecton
Copy link
Member Author

cecton commented Sep 27, 2023

What prevents you from doing Callback<&I, O>

It forces me to provide the lifetime which I have to put in the struct etc... but it's actually not necessary because it's an argument of a function but Rust does not know that

@cecton
Copy link
Member Author

cecton commented Sep 27, 2023

What prevents you from doing Callback<&I, O>

Ah, I forgot to include the important bit from @kirillsemyonkin lol :

doing Callback<&IN, OUT> says that the type should have a 'a which is wrong because its Fn under the hood

@ranile ranile added the A-yew Area: The main yew crate label Sep 29, 2023
Copy link
Member

@futursolo futursolo left a comment

Choose a reason for hiding this comment

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

LGTM!

@futursolo futursolo merged commit 194a1e6 into yewstack:master Sep 29, 2023
22 checks passed
@cecton cecton deleted the callback-ref branch September 30, 2023 07:27
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.

5 participants