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

Redo properties, take 2 #2729

Merged
merged 7 commits into from Jun 21, 2022
Merged

Conversation

WorldSEnder
Copy link
Member

@WorldSEnder WorldSEnder commented Jun 12, 2022

Description

This PR is a follow up to #2369, based on the pattern described in #2369 (comment). It solves the blocker towards inherited props by replacing the manual builder steps tracking the state of the builder (required props) with a trait-guided solution, based on the central HasProp and HasAllProps traits.

I suppose it would also make sense to have another look at #1634. Currently, all props are added in alphabetical order, but with the trait approach, the order shouldn't matter.

Checklist

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

@WorldSEnder WorldSEnder added A-yew Area: The main yew crate A-yew-macro Area: The yew-macro crate labels Jun 12, 2022
github-actions[bot]
github-actions bot previously approved these changes Jun 12, 2022
@github-actions
Copy link

github-actions bot commented Jun 12, 2022

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

https://yew-rs-api--pr2729-prop-inheritance-939zkif9.web.app

(expires Sun, 26 Jun 2022 15:29:29 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Jun 12, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
boids 172.502 172.607 +0.105 +0.061%
contexts 109.604 109.619 +0.015 +0.013%
counter 86.551 86.551 0 0.000%
counter_functional 87.200 87.200 0 0.000%
dyn_create_destroy_apps 89.691 89.691 0 0.000%
file_upload 102.626 102.626 0 0.000%
function_memory_game 166.910 166.910 0 0.000%
function_router 351.658 351.781 +0.123 +0.035%
function_todomvc 161.558 161.558 0 0.000%
futures 226.277 226.277 0 0.000%
game_of_life 107.188 107.188 0 0.000%
inner_html 83.618 83.618 0 0.000%
js_callback 112.829 112.845 +0.016 +0.014%
keyed_list 195.088 195.106 +0.019 +0.010%
mount_point 86.181 86.181 0 0.000%
nested_list 115.513 115.688 +0.175 +0.151%
node_refs 93.596 93.596 0 0.000%
password_strength 1538.306 1538.306 0 0.000%
portals 97.206 97.221 +0.015 +0.015%
router 320.586 320.543 -0.043 -0.013%
simple_ssr 154.478 154.493 +0.016 +0.010%
ssr_router 397.648 397.771 +0.123 +0.031%
suspense 110.519 110.538 +0.020 +0.018%
timer 89.287 89.287 0 0.000%
todomvc 142.614 142.614 0 0.000%
two_apps 87.159 87.159 0 0.000%
web_worker_fib 153.407 153.407 0 0.000%
webgl 87.438 87.438 0 0.000%

✅ None of the examples has changed their size significantly.

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.

Implementation looks good to me. I assume prop inheritance (with Deref/DerefMut?) will come in the future, right?

What if we create a crate like typed-builder (similar to how #2563 creates imut)? That way a type-safe builders can be used outside of Yew as well.

packages/yew/src/html/component/properties.rs Outdated Show resolved Hide resolved
packages/yew/src/html/component/properties.rs Show resolved Hide resolved
packages/yew-macro/src/derive_props/field.rs Outdated Show resolved Hide resolved
packages/yew-macro/src/derive_props/field.rs Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot previously approved these changes Jun 19, 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!

The Deref/DerefMut stuff and prop inheritance can come later. If possible, we should also consider taking it out of Yew and making a separate builder crate for this

@WorldSEnder
Copy link
Member Author

Tagged as technically breaking change, because it removes the old builder-style approach in user code. If anybody actually needs the old style, we can think of a replacement, but I think the props macro or initializing the props directly is often a good work-around.

- Props::Builder::new().x(1).y(2).build()
+ props!{ Props { x: 1, y: 2 } }

I have double-checked the error message stay informative in rust stable, also. I'll merge this in the evening or tomorrow.

@WorldSEnder WorldSEnder merged commit 75bb903 into yewstack:master Jun 21, 2022
@WorldSEnder WorldSEnder deleted the prop-inheritance branch June 21, 2022 20:34
@WorldSEnder WorldSEnder mentioned this pull request Jun 22, 2022
2 tasks
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 A-yew-macro Area: The yew-macro crate breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants