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

Error on refs on components #2863

Merged
merged 3 commits into from Sep 13, 2022
Merged

Conversation

WorldSEnder
Copy link
Member

Description

With #2783, ref was removed from components. With this, this is now a compile time error instead of silently not binding a node during runtime, which could lead to unexpected None returned from get. This also includes a fix to the node_refs example by applying a similar workaround as suggested in the migration notes.

Checklist

  • I have reviewed my own code
  • I have added tests // Actually, there was a test that now has a nice error

r#ref is still allowed, so we reserve the right to reintroduce
the syntax
@github-actions
Copy link

github-actions bot commented Sep 7, 2022

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

https://yew-rs-api--pr2863-error-node-refs-dsl9pdzz.web.app

(expires Mon, 19 Sep 2022 01:53:54 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@WorldSEnder
Copy link
Member Author

The test failures are, as far as I can see, all related to tokio-rs/tokio#4973

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 307.318 308.516 307.900 0.400
Hello World 10 601.914 603.230 602.329 0.444
Function Router 10 2256.231 2275.785 2261.968 5.558
Concurrent Task 10 1008.518 1010.208 1009.196 0.531

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 353.310 355.546 354.639 0.785
Hello World 10 617.096 618.652 617.940 0.399
Function Router 10 2246.122 2273.972 2255.020 8.632
Concurrent Task 10 1008.237 1009.519 1008.900 0.408

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
boids 170.233 170.239 +0.006 +0.003%
communication_child_to_parent 90.466 90.467 +0.001 +0.001%
communication_grandchild_with_grandparent 105.418 105.421 +0.003 +0.003%
communication_grandparent_to_grandchild 101.320 101.319 -0.001 -0.001%
communication_parent_to_child 87.601 87.599 -0.002 -0.002%
contexts 107.849 107.847 -0.002 -0.002%
counter 85.501 85.503 +0.002 +0.002%
counter_functional 86.031 86.030 -0.001 -0.001%
dyn_create_destroy_apps 88.372 88.370 -0.002 -0.002%
file_upload 100.081 100.084 +0.003 +0.003%
function_memory_game 163.779 163.783 +0.004 +0.002%
function_router 348.458 348.471 +0.013 +0.004%
function_todomvc 158.626 158.628 +0.002 +0.001%
futures 221.975 221.988 +0.014 +0.006%
game_of_life 105.739 105.740 +0.001 +0.001%
immutable 181.605 181.604 -0.002 -0.001%
inner_html 82.368 82.369 +0.001 +0.001%
js_callback 111.385 111.383 -0.002 -0.002%
keyed_list 195.571 195.568 -0.003 -0.001%
mount_point 85.122 85.121 -0.001 -0.001%
nested_list 112.596 112.597 +0.001 +0.001%
node_refs 92.851 92.983 +0.133 +0.143%
password_strength 1548.171 1548.170 -0.001 -0.000%
portals 96.287 96.284 -0.003 -0.003%
router 318.126 318.120 -0.006 -0.002%
simple_ssr 152.170 152.179 +0.009 +0.006%
ssr_router 394.003 394.010 +0.007 +0.002%
suspense 109.106 109.111 +0.005 +0.004%
timer 88.317 88.317 0 0.000%
todomvc 139.742 139.743 +0.001 +0.001%
two_apps 86.116 86.115 -0.001 -0.001%
web_worker_fib 152.222 152.229 +0.008 +0.005%
webgl 84.834 84.834 0 0.000%

✅ None of the examples has changed their size significantly.

elem.detach(&root, &parent, false);
scheduler::start_now();
assert!(node_ref.get().is_none(), "components don't have node refs");
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Test removed because this is now a compile error, see component-fail.stderr above.

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.

Could you please also add a test case to demonstrate that r#ref would work as expected?

@WorldSEnder
Copy link
Member Author

Could you please also add a test case to demonstrate that r#ref would work as expected?

That test exists here already: https://github.com/yewstack/yew/blob/master/packages/yew-macro/tests/html_macro/component-pass.rs#L104 - although weirdly enough it uses NodeRef as the type of the r#ref property, which can honestly be confusing for someone reading tests for reference.

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.

Looks good.

@WorldSEnder WorldSEnder merged commit 8eb9b5a into yewstack:master Sep 13, 2022
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.

None yet

2 participants