remove ToHtml trait #3453

ranile · 2023-10-07T10:58:11Z

Description

This trait was a mistake. See the discussion in #3443

This pattern

struct Foo;
impl Into<Html> for Foo {
    fn into(self) -> Html {
        todo!()
    }
}

let foo: Foo = Foo;
// let foo: Html = Foo.into();
// ^^ this fixes the issue
html!(<Comp>{foo}</Comp>)

fails because of missing trait impl for Foo:

error[E0277]: the trait bound `into_prop_value::test::into_html_in_children::Foo: into_prop_value::IntoPropValue<html::component::children::ChildrenRenderer<vnode::VNode>>` is not satisfied
   --> packages/yew/src/html/conversion/into_prop_value.rs:487:26
    |
487 |             html!(<Comp>{foo}</Comp>)
    |                    ----  ^^^ the trait `into_prop_value::IntoPropValue<html::component::children::ChildrenRenderer<vnode::VNode>>` is not implemented for `into_prop_value::test::into_html_in_children::Foo`
    |                    |
    |                    required by a bound introduced by this call

This blanket implementation is not possible

impl<T: Into<VNode>> IntoPropValue<ChildrenRenderer<VNode>> for T {
    fn into_prop_value(self) -> ChildrenRenderer<VNode> {
        todo!()
    }
}

because this one exists:

impl<T> IntoPropValue<T> for T {
    #[inline]
    fn into_prop_value(self) -> T {
        self
    }
}

#3431 makes VNode cheap to clone so it's Into::into isn't an issue

Checklist

I have reviewed my own code
I have added tests

github-actions · 2023-10-07T11:02:23Z

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

https://yew-rs--pr3453-rm-to-html-z2668i6x.web.app

_{(expires Sat, 04 Nov 2023 09:57:34 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

cecton

Ok now I get it why said making html cheap to clone would fix the issue. It's brilliant! I'm sorry I haven't look at the benchmarking yet...

cecton · 2023-10-07T11:03:57Z

packages/yew/src/html/conversion/into_prop_value.rs

+            let attr_value = AttrValue::from("foo");
+
+            let _ = html! { <Child>{&attr_value}</Child> };


Oh it would fix that one too? Excellent!

github-actions · 2023-10-07T11:05:50Z

Size Comparison

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
async_clock	103.382	103.360	-0.021	-0.021%
boids	176.938	176.817	-0.120	-0.068%
communication_child_to_parent	96.095	95.831	-0.264	-0.274%
communication_grandchild_with_grandparent	109.183	109.030	-0.152	-0.140%
communication_grandparent_to_grandchild	105.550	105.435	-0.115	-0.109%
communication_parent_to_child	93.430	93.314	-0.115	-0.123%
contexts	112.853	112.714	-0.139	-0.123%
counter	90.117	90.002	-0.115	-0.128%
counter_functional	90.790	90.663	-0.127	-0.140%
dyn_create_destroy_apps	93.336	93.225	-0.111	-0.119%
file_upload	104.505	104.261	-0.244	-0.234%
function_memory_game	175.716	175.886	+0.170	+0.097%
function_router	353.717	353.806	+0.089	+0.025%
function_todomvc	164.462	164.230	-0.231	-0.141%
futures	232.295	232.174	-0.121	-0.052%
game_of_life	113.244	113.148	-0.096	-0.085%
immutable	190.513	190.273	-0.239	-0.126%
inner_html	86.320	86.320	0	0.000%
js_callback	114.032	113.781	-0.251	-0.220%
keyed_list	202.912	202.673	-0.239	-0.118%
mount_point	89.442	89.213	-0.229	-0.257%
nested_list	117.040	117.399	+0.359	+0.307%
node_refs	97.140	96.923	-0.217	-0.223%
password_strength	1754.215	1753.925	-0.290	-0.017%
portals	98.325	98.070	-0.255	-0.259%
router	321.264	322.340	+1.076	+0.335%
simple_ssr	144.825	144.692	-0.133	-0.092%
ssr_router	391.717	391.806	+0.089	+0.023%
suspense	120.244	120.140	-0.104	-0.087%
timer	92.627	92.549	-0.078	-0.084%
timer_functional	101.403	101.335	-0.068	-0.067%
todomvc	144.583	144.317	-0.266	-0.184%
two_apps	90.244	90.062	-0.182	-0.201%
web_worker_fib	139.044	138.945	-0.099	-0.071%
web_worker_prime	189.821	189.679	-0.143	-0.075%
webgl	88.930	88.930	0	0.000%

✅ None of the examples has changed their size significantly.

github-actions · 2023-10-07T11:19:57Z

Benchmark - SSR

Yew Master

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	465.047	477.287	471.199	4.063
Hello World	10	763.323	781.832	769.064	5.370
Function Router	10	2601.714	2646.707	2615.330	14.618
Concurrent Task	10	1009.291	1010.684	1009.963	0.484
Many Providers	10	1751.501	1776.276	1758.853	9.066

Pull Request

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	459.536	469.039	463.271	2.883
Hello World	10	760.131	780.919	771.674	6.944
Function Router	10	2612.421	2629.206	2621.200	6.168
Concurrent Task	10	1009.037	1010.614	1009.989	0.540
Many Providers	10	1761.937	1788.247	1776.527	7.712

futursolo · 2023-10-07T12:09:51Z

I am not opinionated on how we convert a T into Html.
However, I recall the rationale of adding ToHtml is to avoid implementing IntoPropValue<Html>, which I think is not ideal.

If Into<Html> can be made to work like ToHtml, I am in favour of this change since that's a more standardised way to do things.

#3431 makes VNode cheap to clone so it's Into::into isn't an issue

FYI, I think VNodes created by html! is already cheap to clone because it imposes a VList at the top level of a component children, which already Rc'ed. So #3431 is not a prerequisite of this pull request when it comes to performance consideration. #3431 should still matter, as children may not result in a VList if it only has 1 child.

cecton · 2023-10-07T12:20:40Z

However, I recall the rationale of adding ToHtml is to avoid implementing IntoPropValue<Html>, which I think is not ideal.

Can you share more details on that (link on a previous conversation maybe)?

futursolo · 2023-10-07T12:35:22Z

Can you share more details on that (link on a previous conversation maybe)?

There wasn't a discussion.
Because the children prop is now a standard prop for 1 child (goes through IntoPropValue). This means that for components with 1 child, in order to accept a type as Html, you need to implement IntoPropValue<Html>.
I wished to avoid that scenario, so I added ToHtml. (Because Into<T> is implemented for T, that conflicts with <T: Into<Html>> IntoPropValue<Html> for T. ToHtml is not implemented for Html, so it avoids the trait conflict.)

The pull request was #3289.

Into<Html> and Display can't be implemented on the same type

This reverts commit 52f3c1f. These impls are fine now

github-actions · 2023-10-27T16:49:09Z

Benchmark - core

Yew Master

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  5.452 ns      │ 6.737 ns      │ 5.473 ns      │ 5.485 ns      │ 100     │ 1000000000

Pull Request

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  5.779 ns      │ 5.963 ns      │ 5.864 ns      │ 5.865 ns      │ 100     │ 1000000000

cecton · 2023-10-28T08:16:24Z

(I'm on it, please don't merge too fast)

(There is more work to do but it's complicated so I will do it in another PR)

cecton · 2023-10-28T09:45:08Z

examples/timer/src/main.rs

@@ -139,7 +139,7 @@ impl Component for App {
                        { &self.time }
                    </div>
                    <div id="messages">
-                        { for self.messages.iter().map(|message| html! { <p>{ message }</p> }) }
+                        { for self.messages.iter().map(|message| html! { <p>{ *message }</p> }) }


I'm going to fix that in a separate PR.

Related to yewstack/implicit-clone#37

cecton

I want to do more changes but it will be a bit too extensive so I think I will make a separate PR after this one is merged.

ranile · 2023-10-28T11:11:07Z

I'm going to go ahead and merge this so you can work on your PR

cecton · 2023-11-01T14:21:20Z

packages/yew/src/html/component/children.rs

    /// children.map(|children| {
    ///     html! {
    ///         <div class={classes!("container")}>
-    ///             {children}
+    ///             {children.clone()}


That one too looks weird. I will fix it in my future PR

* remove ToHtml trait * re-add display impls * make Vec::clone expilit * fix doc * fix conflicting impls Into<Html> and Display can't be implemented on the same type * update docs * blanket impl won't work here * bring back `Vec<VNode>: IntoPropValue<VNode>` * macro tests * Revert "fix conflicting impls" This reverts commit 52f3c1f. These impls are fine now * make examples compile * .clone() should be before .into() * Rc VList * Make use of ImplicitClone and AttrValue in example (There is more work to do but it's complicated so I will do it in another PR) * Impl ImplicitClone on VChild --------- Co-authored-by: Cecile Tonglet <[email protected]>

ranile added breaking change A-yew Area: The main yew crate labels Oct 7, 2023

ranile requested review from cecton and futursolo October 7, 2023 10:58