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

Reentrant event listeners #3037

Merged
merged 2 commits into from
Dec 21, 2022
Merged

Conversation

WorldSEnder
Copy link
Member

Description

Fixes #2989 and allows for listeners to trigger events that will reenter yew's event handling (which might trigger further event handlers or exit early).

Checklist

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

@github-actions
Copy link

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

https://yew-rs-api--pr3037-reentrent-listeners-wp0miw5c.web.app

(expires Sun, 18 Dec 2022 19:58:51 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 376.578 377.173 376.784 0.187
Hello World 10 677.495 688.191 683.390 3.959
Function Router 10 2332.073 2368.179 2353.657 11.883
Concurrent Task 10 1007.615 1009.287 1008.604 0.601

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 376.642 377.062 376.831 0.130
Hello World 10 721.391 726.454 724.252 1.563
Function Router 10 2369.398 2415.404 2394.885 11.928
Concurrent Task 10 1007.095 1009.523 1008.410 0.699

@github-actions
Copy link

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 108.256 108.290 +0.034 +0.032%
boids 172.295 172.329 +0.034 +0.020%
communication_child_to_parent 92.356 92.390 +0.033 +0.036%
communication_grandchild_with_grandparent 106.805 106.737 -0.067 -0.063%
communication_grandparent_to_grandchild 102.653 102.585 -0.068 -0.067%
communication_parent_to_child 89.447 89.479 +0.032 +0.036%
contexts 109.328 109.261 -0.067 -0.062%
counter 87.385 87.418 +0.033 +0.038%
counter_functional 87.797 87.729 -0.067 -0.077%
dyn_create_destroy_apps 90.234 90.267 +0.032 +0.036%
file_upload 101.706 101.890 +0.184 +0.181%
function_memory_game 166.479 166.410 -0.068 -0.041%
function_router 350.077 350.236 +0.159 +0.045%
function_todomvc 161.415 161.348 -0.067 -0.042%
futures 226.534 226.572 +0.038 +0.017%
game_of_life 108.143 108.178 +0.035 +0.033%
immutable 184.000 183.922 -0.078 -0.042%
inner_html 83.807 83.838 +0.031 +0.037%
js_callback 112.994 112.938 -0.057 -0.050%
keyed_list 197.807 197.842 +0.035 +0.018%
mount_point 86.956 86.987 +0.031 +0.036%
nested_list 114.938 114.979 +0.040 +0.035%
node_refs 94.873 94.902 +0.029 +0.031%
password_strength 1552.854 1552.791 -0.062 -0.004%
portals 98.149 98.179 +0.029 +0.030%
router 320.163 320.346 +0.183 +0.057%
simple_ssr 153.059 152.995 -0.063 -0.041%
ssr_router 394.539 394.729 +0.190 +0.048%
suspense 110.619 110.555 -0.064 -0.058%
timer 90.279 90.312 +0.033 +0.037%
todomvc 142.749 142.782 +0.033 +0.023%
two_apps 88.016 88.050 +0.034 +0.039%
web_worker_fib 154.375 154.409 +0.034 +0.022%
webgl 86.507 86.539 +0.032 +0.037%

✅ None of the examples has changed their size significantly.

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.

Recursively calling FnMut should not compile if it is pure Rust, but we do not use captured mutable reference internally and Callback is also Fn. Looks good to me.

Copy link
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

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

I would've preferred to have this solved in gloo but I ran into some issues when I tried to do that. This PR looks good to me

There's something I was experimenting with, not sure how ties with here: moving the event listeners registration to JS. Instead of a Rust hash map storing the listeners and calling them, a JavaScript object will. I haven't benchmarked anything and don't have anything concrete enough to turn into a PR; just a thought I had.

@WorldSEnder WorldSEnder merged commit aebd225 into yewstack:master Dec 21, 2022
@WorldSEnder WorldSEnder deleted the reentrent-listeners branch December 21, 2022 01:57
@WorldSEnder WorldSEnder added the A-yew Area: The main yew crate label Jan 9, 2023
WorldSEnder added a commit to WorldSEnder/yew that referenced this pull request Jan 10, 2023
* add test case for reentrent event listeners
* use Fn to allow reentrent event listeners
WorldSEnder added a commit that referenced this pull request Jan 19, 2023
* add test case for reentrent event listeners
* use Fn to allow reentrent event listeners
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.

Uncaught error message since v0.20 upgrade
3 participants