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

Fixed NodeRef not being implicitly cloned with components #2775

Merged
merged 4 commits into from Jul 7, 2022

Conversation

wdcocq
Copy link
Contributor

@wdcocq wdcocq commented Jul 6, 2022

Description

Passing a &NodeRef to a html component ref attribute would throw an error whereas with a html element it wouldn't.
A html component didn't use IntoPropValue on the ref attribute and thus didn't use ImplicitClone.

I've made
packages/yew-macro/src/html_tree/html_component.rs
more inline with how the ref and key attributes are implemented in
packages/yew-macro/src/html_tree/html_element.rs
To make subtle differences between the implementations clearer (e.g. notice the missing optimize_literals() on the key value before)

Checklist

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

@github-actions
Copy link

github-actions bot commented Jul 6, 2022

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

https://yew-rs-api--pr2775-noderef-implicitclon-9fzorf8n.web.app

(expires Wed, 13 Jul 2022 15:38:15 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Jul 6, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
boids 173.043 173.043 0 0.000%
contexts 109.944 109.944 0 0.000%
counter 87.015 87.015 0 0.000%
counter_functional 87.664 87.664 0 0.000%
dyn_create_destroy_apps 90.123 90.123 0 0.000%
file_upload 103.128 103.128 0 0.000%
function_memory_game 167.434 167.434 0 0.000%
function_router 352.430 352.430 0 0.000%
function_todomvc 162.119 162.119 0 0.000%
futures 225.897 225.897 0 0.000%
game_of_life 107.652 107.652 0 0.000%
immutable 208.870 208.870 0 0.000%
inner_html 84.076 84.076 0 0.000%
js_callback 113.175 113.175 0 0.000%
keyed_list 195.715 195.715 0 0.000%
mount_point 86.647 86.647 0 0.000%
nested_list 116.094 116.094 0 0.000%
node_refs 94.081 94.081 0 0.000%
password_strength 1546.331 1546.331 0 0.000%
portals 97.669 97.669 0 0.000%
router 321.237 321.237 0 0.000%
simple_ssr 154.750 154.750 0 0.000%
ssr_router 398.634 398.634 0 0.000%
suspense 110.816 110.816 0 0.000%
timer 89.700 89.700 0 0.000%
todomvc 143.155 143.155 0 0.000%
two_apps 87.658 87.658 0 0.000%
web_worker_fib 153.865 153.865 0 0.000%
webgl 87.896 87.896 0 0.000%

✅ None of the examples has changed their size significantly.

@wdcocq
Copy link
Contributor Author

wdcocq commented Jul 6, 2022

As discussed with @WorldSEnder on discord, I've moved the code to SpecialProps so it can be reused

Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

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

This has tests, does what it advertises and cleans up inconsistencies (even removing lines overall). Looks good to me 🎉

@WorldSEnder WorldSEnder added the A-yew Area: The main yew crate label Jul 6, 2022
Copy link
Member

@hamza1311 hamza1311 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 to me. Until #2505 is resolved, this is a nice improvement

@hamza1311 hamza1311 merged commit 5570710 into yewstack:master Jul 7, 2022
@wdcocq wdcocq deleted the noderef-implicitclone-components branch July 7, 2022 16:12
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.

None yet

3 participants