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

Allow any type to be used as Children (take 2) #3289

Merged
merged 31 commits into from
Jun 11, 2023

Conversation

futursolo
Copy link
Member

@futursolo futursolo commented Jun 3, 2023

Description

Partially supersedes #3042.
(There are some additional optimisation in the original PR, so I want to preserve the PR draft instead of closing it)

This pull request makes the following changes:

  1. Allowing any type to be passed as children as long as IntoPropValue<T> for children with type T.
    E.g.: One can now use render props as children:

    html! {
        <Comp>
            {|p: RenderProps| html!{<>{"Hello, "}{p.name}</>}}
        </Comp>
    }
  2. Added a ToHtml trait. Previously std::fmt::Display was the blanket implementation when a variable is referenced like {item}, this only allows {item} to be converted to VText. Renderable allows a variable to be converted to a VNode, which gives user greater control.

  3. Users can now choose between Children, ChildrenWithProps(ChildrenRenderer<VChild<T>>)
    and Html for html-like Children. As Htmls created by html! is Rc'd, it is faster to use Html when you don't need specific features from Children and ChildrenWithProps.

  4. When a component has only 1 child, it does not pass through an iterator and Vec.

  5. VTag now holds VNode as its children type. This optimises for single child VTag by avoiding an allocation of single child VList.

This new implementation not only allows more types to be used as children, but also reduces code size and is faster.

Checklist

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

github-actions[bot]
github-actions bot previously approved these changes Jun 3, 2023
@github-actions
Copy link

github-actions bot commented Jun 3, 2023

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

https://yew-rs--pr3289-any-children-wrt96p3l.web.app

(expires Sat, 17 Jun 2023 13:05:16 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Jun 3, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 104.027 103.119 -0.908 -0.873%
boids 177.417 176.696 -0.721 -0.406%
communication_child_to_parent 96.902 95.781 -1.121 -1.157%
communication_grandchild_with_grandparent 109.604 109.838 +0.234 +0.214%
communication_grandparent_to_grandchild 105.703 106.322 +0.619 +0.586%
communication_parent_to_child 94.017 93.111 -0.905 -0.963%
contexts 110.692 111.853 +1.160 +1.048%
counter 90.253 89.611 -0.642 -0.711%
counter_functional 90.729 90.390 -0.339 -0.373%
dyn_create_destroy_apps 93.112 92.761 -0.352 -0.378%
file_upload 104.446 103.765 -0.682 -0.653%
function_memory_game 176.521 175.130 -1.392 -0.788%
function_router 349.635 346.331 -3.304 -0.945%
function_todomvc 164.782 163.723 -1.060 -0.643%
futures 227.538 227.683 +0.145 +0.064%
game_of_life 113.052 112.207 -0.845 -0.747%
immutable 190.416 188.901 -1.515 -0.795%
inner_html 86.809 86.389 -0.420 -0.484%
js_callback 114.744 113.857 -0.887 -0.773%
keyed_list 204.797 202.085 -2.712 -1.324%
mount_point 89.933 89.578 -0.354 -0.394%
nested_list 115.313 114.465 -0.849 -0.736%
node_refs 97.721 96.632 -1.089 -1.114%
password_strength 1589.795 1589.260 -0.535 -0.034%
portals 99.127 99.100 -0.027 -0.028%
router 315.734 312.195 -3.539 -1.121%
simple_ssr 146.230 145.125 -1.105 -0.756%
ssr_router 387.476 383.702 -3.773 -0.974%
suspense 112.170 111.172 -0.998 -0.890%
timer 93.050 92.241 -0.809 -0.869%
timer_functional 101.822 100.916 -0.906 -0.890%
todomvc 144.887 143.967 -0.920 -0.635%
two_apps 90.892 90.258 -0.634 -0.697%
web_worker_fib 155.599 154.972 -0.627 -0.403%
webgl 89.323 88.846 -0.478 -0.535%

⚠️ The following examples have changed their size significantly:

examples master (KB) pull request (KB) diff (KB) diff (%)
communication_child_to_parent 96.902 95.781 -1.121 -1.157%
contexts 110.692 111.853 +1.160 +1.048%
keyed_list 204.797 202.085 -2.712 -1.324%
node_refs 97.721 96.632 -1.089 -1.114%
router 315.734 312.195 -3.539 -1.121%

github-actions[bot]
github-actions bot previously approved these changes Jun 3, 2023
@github-actions
Copy link

github-actions bot commented Jun 3, 2023

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 319.245 320.168 319.531 0.317
Hello World 10 657.955 661.203 658.924 1.265
Function Router 10 2212.318 2218.537 2215.432 2.171
Concurrent Task 10 1007.491 1008.923 1008.270 0.469
Many Providers 10 1699.065 1721.156 1710.257 7.489

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 338.784 339.989 339.486 0.346
Hello World 10 648.297 668.178 656.372 6.341
Function Router 10 2093.251 2113.675 2101.398 6.992
Concurrent Task 10 1007.108 1008.171 1007.568 0.345
Many Providers 10 1449.720 1468.142 1456.331 5.535

github-actions[bot]
github-actions bot previously approved these changes Jun 3, 2023
github-actions[bot]
github-actions bot previously approved these changes Jun 3, 2023
github-actions[bot]
github-actions bot previously approved these changes Jun 3, 2023
github-actions[bot]
github-actions bot previously approved these changes Jun 3, 2023
github-actions[bot]
github-actions bot previously approved these changes Jun 3, 2023
github-actions[bot]
github-actions bot previously approved these changes Jun 3, 2023
@futursolo futursolo added breaking change A-yew Area: The main yew crate A-yew-macro Area: The yew-macro crate labels Jun 3, 2023
github-actions[bot]
github-actions bot previously approved these changes Jun 3, 2023
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.

Thanks for taking the time to implement it!

packages/yew-macro/src/html_tree/mod.rs Outdated Show resolved Hide resolved
Comment on lines -312 to +318
{header}
{for header}
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't VNode be also fine for this? {vnode} should work fine

packages/yew/src/server_renderer.rs Show resolved Hide resolved
packages/yew/src/virtual_dom/renderable.rs Outdated Show resolved Hide resolved
packages/yew/src/virtual_dom/renderable.rs Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot previously approved these changes Jun 10, 2023
@futursolo futursolo requested a review from ranile June 10, 2023 13:11
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.

Looks good to me!

One question though: I don't see any error messages in tests that say "ToHtml is not implemented". Is that supposed to be like that because of From conversions? Or are failing tests missing, in which case we should add them?

@futursolo
Copy link
Member Author

Looks good to me!

One question though: I don't see any error messages in tests that say "ToHtml is not implemented". Is that supposed to be like that because of From conversions? Or are failing tests missing, in which case we should add them?

I would think this as expected.
This is due to ToHtml is never directly required. The conversion goes through IntoPropValue<VNode> and then in turn require ToHtml via a blanket implementation.

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