Struggling with ways to convert someting into `Html`. #3443

ctron · 2023-10-04T11:05:17Z

Problem

Steps To Reproduce
Steps to reproduce the behavior:

    #[test]
    fn test_to_html_to_children_comp_compiles() {
        use crate::prelude::*;

        struct Foo;

        impl ToHtml for Foo {
            fn to_html(&self) -> Html {
                html!()
            }
        }

        #[derive(PartialEq, Properties)]
        pub struct FooProps {
            pub children: Children,
        }

        #[function_component(Comp)]
        fn comp(props: &FooProps) -> Html {
            html!()
        }

        let _ = html!(<Comp>{Foo}</Comp>);
    }

Expected behavior

In the past it was possible to implement From<Foo> for Html to use something in the html macro, like this:

struct Foo;
html! (
 <SomeComponent>{ Foo }</SomeComponent>
)

Now there seem to be two new variants: ToHtml, and IntoPropValue<ChildrenRenderer<Vnode>>. However, it's not always clear when to use what and why sometimes ToHtml, doesn't work.

Environment:

Yew version: v0.21.0.
Rust version: 1.17.1

Questionnaire

I'm interested in fixing this myself but don't know where to start
I would like to fix and I have a solution
I don't have time to fix this right now, but maybe later

The text was updated successfully, but these errors were encountered:

ctron · 2023-10-04T11:27:34Z

I added a reproducer.

ctron · 2023-10-04T11:34:59Z

The absolute downside of moving away from From<T> for Html is that now I need to implement three traits for the same effect: From<T> for Html, IntoPropValue<ChildrenRenderer<VNode>>, and ToHtml. Depending on the context I use this.

I think either From<T> for Html or ToHtml would be ok. But the requirement for IntoPropValue<ChildrenRenderer<VNode>> is a bit much.

hamza1311 · 2023-10-04T15:37:08Z

What prevents you from just using ToHtml? Html can be passed as children to a component now. I don't see a case where ChildrenRenderer is needed

I think an implementation:

impl<T: Into<Html>> ToHtml for T {}

could be helpful.

CC: @futursolo

ctron · 2023-10-04T16:09:28Z

In the past all children was the type Children. Or ChildrenWithProperties<T>.

All the examples I found in the docs still follow this pattern. If that has changed, the maybe the docs need some update. As well a the pattern of using ChildrenWithProperties. Or a specific type of children.

hamza1311 · 2023-10-04T16:13:21Z

That was changed in the latest release 0.21 and docs were updated to reflect the change. Are there any docs you're seeing that have not been updated?

ctron · 2023-10-04T16:14:45Z

I was just looking for them. Could it be that the switch, defaulting to the 0.21 docs , was made a bit after the release? I might have been running into the old docs.

ctron · 2023-10-04T16:16:30Z

At least in the module yew::html::component::children I can find the old examples. As well as the Children type itself. Wouldn't it make sense to deprecate it, having a proper message.

hamza1311 · 2023-10-04T16:16:55Z

Docs default to 0.21 when the release is made. You can see the updated children docs: https://yew.rs/docs/concepts/function-components/children

Any type, including Html, can be passed as children. This was also mentioned in the release blog

ctron · 2023-10-04T16:19:48Z

Docs default to 0.21 when the release is made.

I am pretty sure I saw 0.20 docs by default after the release was made.

Any type, including Html, can be passed as children. This was also mentioned in the release blog

However, there was nothing in the migration guide: https://yew.rs/docs/migration-guides/yew/from-0_20_0-to-0_21_0 … so there should at least be some "also see the blog post" link in there.

hamza1311 · 2023-10-04T16:26:51Z

Docs default to 0.21 when the release is made.

I am pretty sure I saw 0.20 docs by default after the release was made.

Hmm, not sure why that is. Could be caching?

Also, the migration guide is for breaking changes. I'm not sure where exactly this kind of change would be mentioned. I agree that it be mentioned, just not sure where. Perhaps we could add a section for non breaking changes in the migration guide? Pinging @futursolo as they were the one who made this change.

ctron · 2023-10-04T16:28:56Z

I would really call that a breaking change though.

What no longer works with Html is a pattern like this:

                { for props.children.iter().map(|item| html!(
                    <div class="pf-v5-c-accordion__expandable-content-body">
                        { item }
                    </div>
                )) }

ctron · 2023-10-04T16:29:20Z

Or like this:

if !props.children.is_empty() { }

hamza1311 · 2023-10-04T16:36:01Z

I don't think this is a breaking change as no existing code breaks. You're allowed to still use ChildrenRenderer if it's needed.

You can pass any type as children now. For your case, VList exists (in addition to children). html_nested! can be used to construct any variant of VNode without wrapping it in VNode

ctron · 2023-10-04T16:41:33Z

Right, but when I need to use Children, I also need to implement the IntoProp trait … so I am back to the problem above. All in all, not a step forward IMHO.

Besides, I does break current code, because Into<Html> no longer works.

hamza1311 · 2023-10-04T16:43:16Z

I wonder if we can add blanket trait implantations for this

ctron · 2023-10-04T16:51:18Z

Using VList for children I indeed can iterate over it. But I can no longer use the html macro. Because the html macro will generate all kind of nodes, not necessarily VList.

hamza1311 · 2023-10-04T16:52:32Z

That is what html_nested! macro is for

ctron · 2023-10-04T16:54:12Z

Yea, but assume I have something like this:

html!(
<Foo>
  <Bar>
    <Baz>
    </Baz>
  </Bar>
</Foo>
)

If I want Foo to be aware of the number of children, using VList, then I need to inject html_nested now?

hamza1311 · 2023-10-04T17:57:18Z

I created #3444 to allow that pattern

flumm · 2023-10-05T08:31:46Z

hi, we run into a similar (the same?) issue, we often implemented Into<Html> (or Into<VNode>) on the properties struct, so that we can use a builder pattern for them and still use them in the html!{} macro. Since upgrading to 0.21 this does not work anymore.

I played around with implementing a blanket impl block for this, but ran into issues with conflicting implementations (either with the other ToHtml implementations or with the IntoPropValue implementations) so maybe someone who has a better understanding of how they play together can whip something up?

flumm · 2023-10-06T07:31:47Z

Hi,

the bug was closed, but i think there is still a valid concern here (Please tell if i should open (a) new bug(s).
We have an issue with From<...> for Html and the ToHtml trait that worked before and does not now.
A short example that worked in 0.20 and doesn't anymore:

use yew::prelude::*;

struct Print {
    text: String,
}

impl Print {
    fn new(text: String) -> Self {
        Self { text }
    }
}

impl From<Print> for Html {
    fn from(val: Print) -> Self {
        html! {<>{"Hello "}{val.text}</>}
    }
}

#[function_component]
fn App() -> Html {
    let text = Print::new("World".to_owned());
    html! { <div>{text}</div> }
}

fn main() {
    yew::Renderer::<App>::new().render();
}

Implementing ToHtml for that is often unpractical, since you have to copy/clone the properties to implement to_html even if you just want it to be able to convert it without copying/cloning anything

also maybe related is the following issue:

use yew::prelude::*;

fn print<T: ToHtml>(text: T) -> Html {
    html! {<div>{"Hello "}{text}</div>}
}

#[function_component]
fn App() -> Html {
    let vnode = html! {"test"};
    print(vnode)
}

fn main() {
    yew::Renderer::<App>::new().render();
}

here we get the error that VNode does not implement ToHtml which is also super unpractical since sometimes you want to have a function that wants to copy/clone and consume the parameter (depending on the context)

futursolo · 2023-10-06T12:11:52Z

Implementing ToHtml for that is often unpractical, since you have to copy/clone the properties to implement to_html even if you just want it to be able to convert it without copying/cloning anything

There are 2 methods on ToHtml.
1 is to_html and the other one is into_html. into_html is provided with a blanket implementation of to_html. You can implement into_html as well if you think you are able to provide more efficient implementation for fn(Self) -> Html.

ctron · 2023-10-06T12:30:55Z

I think one of the issues with the new approach is, that one needs to implement many ways now. Depending on the context. Assuming the following for example:

  Icon(Icon),
  Text(String),
  Custom(Html),
}

What I want is to just use:

// either using From, or some custom trait.
impl From<Label> for Html {
}

fn component() -> Html {
  let label : Label;
  html!(<div>{&label}</div>)
}

What I need to do is:

impl ToHtml for Label {
  fn to_html() {}
  fn into_html() {} // because I want to avoid cloning
}

// because some people might want to use `Children`.
impl IntoPropValue<ChildrenRenderer<VNode>> for Label {
}

fn component() -> Html {
  let label : Label;
  html!(<div>{label.clone()}</div>) // second clone, unless I implement into_html()
}

So it more work, to get to the same result.

flumm · 2023-10-06T12:52:17Z

Implementing ToHtml for that is often unpractical, since you have to copy/clone the properties to implement to_html even if you just want it to be able to convert it without copying/cloning anything

There are 2 methods on ToHtml. 1 is to_html and the other one is into_html. into_html is provided with a blanket implementation of to_html. You can implement into_html as well if you think you are able to provide more efficient implementation for fn(Self) -> Html.

true but i have to implement a copying/cloning version now, where as before i did not (as @ctron correctly wrote)
wouldn't it be possible to split the trait into two? e.g. an ToHtml trait with to_html that copies/clones and an IntoHtml trait with into_html that consumes (or ideally just reuse Into<Html> for the second part) ? That way i could just implement the consuming part if i want to avoid it being copied

I'm sorry that i don't have a very deep understanding of the ToHtml/IntoPropValue/ChildRenderer traits to make a more concrete suggestion, but the current way seems a bit unwieldy to use for some use-cases.

(currently we work around the issue by requiring 'Clone' on the types we use that for and use

impl ToHtml for XXX {
    fn to_html(&self) -> Html {
        self.clone().into_html()
    }

    fn into_html(self) -> Html {
        self.into()
    }
}

where .into() leads back to our From/Into impls

hamza1311 · 2023-10-06T13:34:04Z

@futursolo when you left in Children, was there any reason other than backwards compatibility? If not, we can deprecate that and point everyone to use Html or any of its variants as needed@futursolo when you left in Children, was there any reason other than backwards compatibility? If not, we can deprecate that and point everyone to use Html or any of its variants as needed

I'm starting to think ToHtml was a mistake. With #3431, the cost of cloning Html goes down greatly so perhaps we could switch to Into<Html>

hamza1311 · 2023-10-06T13:38:29Z

wouldn't it be possible to split the trait into two? e.g. an ToHtml trait with to_html that copies/clones and an IntoHtml trait with into_html that consumes (or ideally just reuse Into<Html> for the second part) ? That way i could just implement the consuming part if i want to avoid it being copied

No, not on stable at least, as far as I know. That would need specialization so we can have a blanket impl of IntoHtml and be allowed to specialize it when possible

cecton · 2023-10-07T09:36:52Z

I have given a try at migrating to help understand the issue here and this is what I found:

&AttrValue cannot be converted automatically to html anymore:

error[E0277]: the trait bound `&implicit_clone::unsync::IString: IntoPropValue<ChildrenRenderer<VNode>>` is not satisfied
  --> yewprint-doc/src/text/example.rs:15:18
   |
14 |             <Text ellipsize={props.ellipsize}>
   |              ---- required by a bound introduced by this call
15 |                 {&props.text}
   |                  ^^^^^^^^^^^ the trait `IntoPropValue<ChildrenRenderer<VNode>>` is not implemented for `&implicit_clone::unsync::IString`
   |
note: required by a bound in `yewprint::TextPropsBuilder::children`
  --> /home/cecile/repos/yewprint/src/text.rs:4:28
   |
4  | #[derive(Clone, PartialEq, Properties)]
   |                            ^^^^^^^^^^ required by this bound in `TextPropsBuilder::children`
...
9  |     pub children: Children,
   |         -------- required by a bound in this associated function
   = note: this error originates in the derive macro `Properties` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider dereferencing here
   |
15 |                 {&*props.text}
   |                   +
help: consider removing the leading `&`-reference
   |
15 -                 {&props.text}
15 +                 {props.text}
   |

For more information about this error, try `rustc --explain E0277`.
error: could not compile `yewprint-doc` (lib) due to previous error

I had to do the following change:

diff --git a/yewprint-doc/src/text/example.rs b/yewprint-doc/src/text/example.rs
index ed83f1c8..eb2f7a67 100644
--- a/yewprint-doc/src/text/example.rs
+++ b/yewprint-doc/src/text/example.rs
@@ -12,7 +12,7 @@ pub fn example(props: &ExampleProps) -> Html {
     html! {
         <div style="width: 150px; height: 20px">
             <Text ellipsize={props.ellipsize}>
-                {&props.text}
+                {&*props.text}
             </Text>
         </div>
     }

I'm quite unhappy (but not too much) with this change because I think AttrValue should be handled in a very transparent way by Yew.

Things that implement ToString/Display cannot be converted to Html directly:

error[E0277]: the trait bound `sv::Value<Ratio>: ToHtml` is not satisfied
  --> xxxxxxxxxx
   |
78 | /                         html! {
79 | |                             <div class={classes!("value")}>
80 | |                             {setback}
   | |                              ^^^^^^^ the trait `ToHtml` is not implemented for `sv::Value<Ratio>`
81 | |                             </div>
82 | |                         },
   | |_________________________- required by a bound introduced by this call
   |
   = help: the following other types implement trait `ToHtml`:
             bool
             char
             isize
             i8
             i16
             i32
             i64
             i128
           and 26 others
   = note: required for `sv::Value<Ratio>` to implement `yew::html::IntoPropValue<VNode>`

I had to do this change:

diff --git a/xxxxx b/xxxxxxxx
index 9f7128b..14413d7 100644
--- a/xxxxxxx
+++ b/xxxxxx
@@ -77,7 +77,7 @@ impl Component for Config {
                         },
                         html! {
                             <div class={classes!("value")}>
-                            {setback}
+                            {setback.to_string()}
                             </div>
                         },
                     )

I never had to implement anything on sv::Value (a struct of mine) itself, this code used to work in yew 0.20. My guess is that it was using the ToString/Display trait to convert my struct sv::Value to html.

Though I must say I'm not unhappy with this change because the conversion to string should be quite intentional. The user should realize that it is something that will require allocation.

I inspected the code around the conversion in Yew and it looks really wrong:

macro_rules! impl_to_html_via_display {
    ($from_ty: ty) => {
        impl ToHtml for $from_ty {
            #[inline(always)]
            fn to_html(&self) -> Html {
                Html::VText(VText::from(self))
            }
        }

        // Mirror ToHtml to Children implementation.
        impl IntoPropValue<ChildrenRenderer<VNode>> for $from_ty {
            #[inline(always)]
            fn into_prop_value(self) -> ChildrenRenderer<VNode> {
                ChildrenRenderer::new(vec![VText::from(self).into()])
            }
        }
    };
}

// These are a selection of types implemented via display.
impl_to_html_via_display!(bool);
impl_to_html_via_display!(char);
impl_to_html_via_display!(String);
impl_to_html_via_display!(&str);
impl_to_html_via_display!(Rc<str>);
impl_to_html_via_display!(Rc<String>);
impl_to_html_via_display!(Arc<str>);
impl_to_html_via_display!(Arc<String>);
impl_to_html_via_display!(AttrValue);
// ...

And in VText:

impl<T: ToString> From<T> for VText {
    fn from(value: T) -> Self {
        VText::new(value.to_string())
    }
}

This means we are allocating a String to convert AttrValue to String and then to VText 🤦‍♀️ that is not efficient.

@hamza1311 @futursolo if you don't mind I'm re-opening this ticket until OP and others are happy or accept the change of behavior. It looks like something needs to be done: either Yew needs to be updated OR our community see and understand why we change things.

(It's not for me, I am not personally much affected by the change of Yew 0.21. The change 1 is a minor annoyance for me, change 2 I think it's actually better to explicitly write down to_string (because it's more explicit) but I don't mind if ToString/Display works again. The change 3 is annoying for me BUT it is unrelated to this issue)

[edit: lots of formatting issue in the post haha xD]

hamza1311 · 2023-10-07T10:01:07Z

I agree with @cecton here. We should do something to mitigate the issues here

We need some blanket implementations that are missing. I'm not sure if we can add them since specialization doesn't exist:

impl<T: Into<VNode>> ToHtml for T {}

Perhaps even removing ChildrenRenderer outright. I don't see where it's needed. Are there any use cases for it?

cecton · 2023-10-07T12:13:47Z

Or like this:

if !props.children.is_empty() { }

Should we add a method .is_empty() on VNode? And also an impl Iterator?

If the VNode is a VList it would be a proxy but if it's anything else it will return true (as not empty) and a single element if using the iterator?

(Also we can yield clones of Html with the iterator since VNode will be cheap to clone. But I guess it's best to include that in the #3431 PR)

futursolo · 2023-10-07T12:17:04Z

Should we add a method .is_empty() on VNode?

I do not think we should be inspecting a VNode. I do not think delivers a very good schematics to users when it comes to some layouts.

e.g.:

For the following layout, should is_empty() return true or false?

html!{
<>
    <></>
</>
}

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

when you left in Children, was there any reason other than backwards compatibility?

No, I just didn't want to introduce a breaking changes that breaks every component that has children.
Maybe that has back-fired.

Perhaps even removing ChildrenRenderer outright. I don't see where it's needed. Are there any use cases for it?

ChildrenRenderer is also used by html! internally.
However, I do not see it as a hard requirement for it still to be the case if we decide to remove Children and ChildrenWithProps.

ctron · 2023-10-07T18:19:11Z

I think that the "is empty" case could also be translated into Option<Html> or maybe Vec<Html> … which feels more natural. And that should work now that children can be any type.

However, a big thing of Rust (and Yew) is type safety. So when a component requires children of a certain type, that should be supported and natural. And in the past, it was a bit tricky on the implementor's side, but did work great for users. So removing ChildrenWithProps without a proper replacement feels wrong to me.

hamza1311 · 2023-10-07T18:33:39Z

I didn't think of ChildrenWithProps. I agree, that type safety is important to keep. I wonder if there's a way to have typed VList or how Vec<VChild<_>> is problematic? For the latter, I don't like the need to create the vec as:

vec! [
    html_nested!(),
    html_nested!()
    // ..
]

but I believe that can be easily mitigated with the help of a macro

futursolo · 2023-10-08T00:42:47Z

I think we can also encourage a separation of data and rendered layout.

let items = vec![
    Item { key: "1", data: "Data 1".to_string() },
    Item { key: "2", data: "Data 2".to_string() },
    Item { key: "3", data: "Data 3".to_string() },
    Item { key: "4", data: "Data 4".to_string() },
];

let render_item = |item: Item<String>| html! { <Comp prop={&item.data} /> };

html! {
    <Table<String>
        {items}
        {render_item}
    />
}

In this example, Table is responsible of fully keying the item and requires a key to be passed to each item.

There is a React example: https://mui.com/x/react-data-grid/getting-started/#demo

ctron added the bug label Oct 4, 2023

hamza1311 mentioned this issue Oct 4, 2023

Allow converting multiple component children to VList #3444

Merged

2 tasks

futursolo closed this as completed in #3444 Oct 5, 2023

cecton reopened this Oct 7, 2023

hamza1311 mentioned this issue Oct 7, 2023

remove ToHtml trait #3453

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Struggling with ways to convert someting into `Html`. #3443

Struggling with ways to convert someting into `Html`. #3443

ctron commented Oct 4, 2023 •

edited

ctron commented Oct 4, 2023

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023 •

edited

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023

ctron commented Oct 4, 2023

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023

ctron commented Oct 4, 2023

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023

ctron commented Oct 4, 2023 •

edited

hamza1311 commented Oct 4, 2023

flumm commented Oct 5, 2023

flumm commented Oct 6, 2023 •

edited

futursolo commented Oct 6, 2023

ctron commented Oct 6, 2023

flumm commented Oct 6, 2023

hamza1311 commented Oct 6, 2023 •

edited

hamza1311 commented Oct 6, 2023

cecton commented Oct 7, 2023 •

edited

hamza1311 commented Oct 7, 2023

cecton commented Oct 7, 2023

futursolo commented Oct 7, 2023

futursolo commented Oct 7, 2023 •

edited

ctron commented Oct 7, 2023

hamza1311 commented Oct 7, 2023

futursolo commented Oct 8, 2023 •

edited

Struggling with ways to convert someting into Html. #3443

Struggling with ways to convert someting into Html. #3443

Comments

ctron commented Oct 4, 2023 • edited

ctron commented Oct 4, 2023

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023 • edited

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023

ctron commented Oct 4, 2023

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023

ctron commented Oct 4, 2023

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023

ctron commented Oct 4, 2023

hamza1311 commented Oct 4, 2023

ctron commented Oct 4, 2023 • edited

hamza1311 commented Oct 4, 2023

flumm commented Oct 5, 2023

flumm commented Oct 6, 2023 • edited

futursolo commented Oct 6, 2023

ctron commented Oct 6, 2023

flumm commented Oct 6, 2023

hamza1311 commented Oct 6, 2023 • edited

hamza1311 commented Oct 6, 2023

cecton commented Oct 7, 2023 • edited

hamza1311 commented Oct 7, 2023

cecton commented Oct 7, 2023

futursolo commented Oct 7, 2023

futursolo commented Oct 7, 2023 • edited

ctron commented Oct 7, 2023

hamza1311 commented Oct 7, 2023

futursolo commented Oct 8, 2023 • edited

Struggling with ways to convert someting into `Html`. #3443

Struggling with ways to convert someting into `Html`. #3443

ctron commented Oct 4, 2023 •

edited

hamza1311 commented Oct 4, 2023 •

edited

ctron commented Oct 4, 2023 •

edited

flumm commented Oct 6, 2023 •

edited

hamza1311 commented Oct 6, 2023 •

edited

cecton commented Oct 7, 2023 •

edited

futursolo commented Oct 7, 2023 •

edited

futursolo commented Oct 8, 2023 •

edited