Skip to content

Redo properties, take 2#2729

Merged
WorldSEnder merged 7 commits into
yewstack:masterfrom
WorldSEnder:prop-inheritance
Jun 21, 2022
Merged

Redo properties, take 2#2729
WorldSEnder merged 7 commits into
yewstack:masterfrom
WorldSEnder:prop-inheritance

Conversation

@WorldSEnder
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

github-actions Bot commented Jun 12, 2022

Size Comparison

Details
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
Copy Markdown
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.

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.

Comment thread packages/yew/src/html/component/properties.rs Outdated
Comment thread packages/yew/src/html/component/properties.rs
Comment thread packages/yew-macro/src/derive_props/field.rs Outdated
Comment thread packages/yew-macro/src/derive_props/field.rs Outdated
github-actions[bot]
github-actions Bot previously approved these changes Jun 19, 2022
Copy link
Copy Markdown
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.

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
Copy Markdown
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.

2 participants