Pass string types to Html props

[cecton]

Description

TODO

Checklist

• [x] I have reviewed my own code
• [x] I have added tests

[github-actions[bot]]

Size Comparison

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
boids	170.449	170.449	0	0.000%
communication_child_to_parent	90.596	90.601	+0.005	+0.005%
communication_grandchild_with_grandparent	105.538	105.541	+0.003	+0.003%
communication_grandparent_to_grandchild	101.431	101.436	+0.005	+0.005%
communication_parent_to_child	87.738	87.737	-0.001	-0.001%
contexts	107.954	107.960	+0.006	+0.005%
counter	85.610	85.611	+0.001	+0.001%
counter_functional	86.138	86.141	+0.003	+0.003%
dyn_create_destroy_apps	88.547	88.544	-0.003	-0.003%
file_upload	100.188	100.188	-0.001	-0.001%
function_memory_game	163.357	163.355	-0.002	-0.001%
function_router	347.846	347.868	+0.022	+0.006%
function_todomvc	158.164	158.172	+0.008	+0.005%
futures	222.484	222.487	+0.003	+0.001%
game_of_life	105.157	105.156	-0.001	-0.001%
immutable	181.864	181.867	+0.003	+0.002%
inner_html	82.478	82.477	-0.001	-0.001%
js_callback	111.599	111.589	-0.010	-0.009%
keyed_list	195.720	195.725	+0.005	+0.002%
mount_point	85.235	85.236	+0.001	+0.001%
nested_list	112.138	112.143	+0.005	+0.004%
node_refs	93.143	93.142	-0.001	-0.001%
password_strength	1548.082	1548.081	-0.001	-0.000%
portals	96.539	96.536	-0.003	-0.003%
router	317.542	317.558	+0.016	+0.005%
simple_ssr	152.456	152.448	-0.008	-0.005%
ssr_router	393.419	393.426	+0.007	+0.002%
suspense	109.417	109.407	-0.010	-0.009%
timer	88.434	88.430	-0.004	-0.004%
todomvc	139.269	139.267	-0.002	-0.001%
two_apps	86.224	86.225	+0.001	+0.001%
web_worker_fib	152.531	152.529	-0.002	-0.001%
webgl	84.915	84.918	+0.003	+0.003%

✅ None of the examples has changed their size significantly.

[ranile]

This seems like a lot of magic for removing a macro invocation. The caller has no idea that a VNode has been constructed and passed to the component. Sounds too much of an implicit conversion to me.

Do we even want to support it? I don't think we should

[cecton]

This seems like a lot of magic for removing a macro invocation.

Allow me to explain.

Currently, an html node can be a string node:

html! {
    <p></p> // paragraph node
}

html! {
    "Text node" // text node
}

As you know I'm working on Yewprint which is a port of Blueprint to Yew. There are a lot of cases where I want to let the user customize the inner (children) element, the "left" element, the "right" element or the "label" element of a component.

For children (inner), you can already pass a string node or an html node easily:

html! {
    <MyComponent>
        <p>Stuff</p>
    </MyComponent>
}

html! {
    <MyComponent>
        {"Stuff"}
    </MyComponent>
}

But for the others you have to html-ify things which is kind of a pain:

html! {
    <MyComponent label={html!(<p>Stuff</p>)}>
        <p>Stuff</p>
    </MyComponent>
}

html! {
    <MyComponent label={html!("Stuff")}>
        {"Stuff"}
    </MyComponent>
}

Let's take for example the Checkbox component and see how they do in TS: https://blueprintjs.com/docs/#core/components/checkbox

You see, the only reason why they made a separate prop for string and for element is because of the typing in TypeScript. It's more of a limitation rather than a thoughtful choice (I believe but correct me if I'm wrong). But in fact, the use in the code is exactly the same:

    return React.createElement(
        tagName,
        { className: classes, style },
        <input {...htmlProps} ref={inputRef} type={type} />,
        <span className={Classes.CONTROL_INDICATOR}>{indicatorChildren}</span>,
        label,
        labelElement,
        children,
    );

With Yew we have the chance to already have this conversion from string to node. All we need to add to have this "complete" is to also allow it in properties. This would allow the user to write this:

html! {
    <MyComponent label={html!(<p>Label Stuff</p>)}>
        <p>Child Stuff</p>
    </MyComponent>
}

html! {
    <MyComponent label="Label Stuff">
        {"Child Stuff"}
    </MyComponent>
}

Without this feature I will have to make the separation label/labelElement like they did with TS... or do like I do now: let the user cast using html!() by themselves (because I don't want to maintain duplicated unnecessary code just to save 7 extra chars for the user).

Now in term of magic I do disagree with you because when looking at Yew's API and seeing that strings are already implicitly converted to text nodes, I only think it's fair to do it for props too. There is an argument to be made (I think that's what you have in mind) about the extra work of constructing a node implicitly and the not obvious impact on performances *but* when you do pass a string to a component that you know will be displayed somehow on the page, you do expect that this text node is going to be created in one way or another.

because I don't want to maintain duplicated unnecessary code just to save 7 extra chars for the user

I'm sure you're going to say: oh so it's not that bad. And no it's definitely not *that* bad. I can live with this. This whole PR is just a QoL improvement a bit like the Option<VNode> automatically casting to VNode. Everything works fine but the API is smoother with those improvements. We need to make sure to not make unreasonable things though, I totally agree with you on that and you're right to challenge my idea here.

[ranile]

The thing is, I'm not aware of any type being converted to VNode implicitly. All the conversions happen between VNode or VNode is not invoked at all. Option<VNode> -> VNode is just optional_html.unwrap_or_default(), there's no implicit type conversion or allocation happening, unlike here.

Also, I don't agree with the TypeScript comparison here.

let x: string | number /*or any*/ = 'str'
x = 12

That's only possible before after type checking there are no types involved. x does not have a type. There's no Rust equivalent to that as Rust is statically typed. The enums are not erased after type checking is done (correct me if I'm wrong here). Even if they were, they are still very much their own types, unlike type script. string | number (or more accurately string | JSX.Element) is not comparable to Rust enum.

---

A better solution to this would have the following impl:

impl From<T: Into<AttrValue>> for VNode {
    fn from(attr_value: T) -> Self {
        VNode::VText(VText::new(attr_value))
    }
}

This can be consumed as:

html! {
    <MyComponent label={"Label Stuff".into()}>
        <p>{"Child Stuff"}</p>
    </MyComponent>
}

Same result but without any macro magic or implicit type conversion involved.

[cecton]

Same result but without any macro magic or implicit type conversion involved.

I don't think I will bother with a QoL improvement to replace the need of html!(...) (7 chars) with ....into() (7 chars) :grimacing: I'd rather close this PR (not to be rude sorry).

there's no implicit type conversion or allocation happening, unlike here

I do have the impression that the implicit allocation bothers you the most. Nevertheless:

1. There will be allocations: <MyComponent> takes a label property which can be a string. The thing I will do with this string is to make it a node. There is no way around this. Check this component for example: https://github.com/yewprint/yewprint/blob/HEAD/src/callout.rs It doesn't allow passing an HTML node at the moment, only AttrValue, the only thing I do with it is wrap it in a <h4>. I mean, when you pass a string to a Yew component, you kinda expect that it will ends up in the dom somehow.
2. We do create nodes from string implicitly already: when you do html! { <p>{"Hello!"}</p> }, the {"Hello"} is converted to a VNode already.
3. It's kinda cheap: if the user pass a AttrValue or any kind of string, it gets converted to VText which uses AttrValue internally. If the user pass a VNode, it's used as is.

That's only possible before after type checking there are no types involved. x does not have a type. There's no Rust equivalent to that as Rust is statically typed. The enums are not erased after type checking is done (correct me if I'm wrong here). Even if they were, they are still very much their own types, unlike type script. string | number (or more accurately string | JSX.Element) is not comparable to Rust enum.

I'm not 100% sure about what you are saying. Are you saying that x has technically no type because in the end it gets transpiled to JS which has no type?

I guess that's technically true but it's quite reductive... The intention of TS is to bring the values of strong typing into JS (which doesn't support this natively). Of course JS does not have enough types for that so it's only a fake strong typing. Nevertheless, a lot of people call TS "strongly typed" probably as a shortcut even if it's not 100% accurate.

My point here is that if you argument is that TS is not statically typed, I think it's irrelevant. What is important about statically typed language is that they are typed. The fact that it is statically done or fake it like TS is just a (somewhat important) detail of implementation.

Let's take Blueprint's toolkit for example. Their entire API is made with the assumption that the user will use TS and rely on typing. And they do go to a great length to accommodate those users even if it is not required for the things to work. I mean, they could remove the property labelElement entirely and let the user deal with the limitations of TS but they don't do that. They want their TS users to have "strong typing".

Either way it doesn't change the fact that they do have label and labelElement only because of a limitation in TS' typing system (that seems to be a fact). The property label is clearly meant for strings and for HTML elements, it is meant that way.

Now about the reason why they went that way, it's probably because they pass the things directly to React.createElement() which takes children that can be of type HTML element or strings. createElement is comparable to creating a VNode and we do, in Yew, also allow creating VNode from strings (html! { "Hello" }).

[ranile]

I'm also skeptical of adding new features, in addition to allocations and magic. It's easy to add features but hard to remove them, if that's needed.

If we were to add this, we should add From implementations for VNode and have a blanket into prop impls for those. Note that I'm not against this QoL improvement, if we're adding it for string types, why not also add it anything that can be converted to VNode?

[cecton]

It's easy to add features but hard to remove them, if that's needed.

It's a very good philosophy. There is that very nice article on the subject: https://programmingisterrible.com/post/139222674273/how-to-write-disposable-code-in-large-systems

I'm just sharing xD it's not at all related to this PR... Though I guess I mindful myself in term of features and I'm kinda lazy anyway. In this case I have been typing a lot of <MyComponent stuff={html!(t!(something => lang)} /> (t! is to generate translated text here). It's getting old.

If we were to add this, we should add From implementations for VNode and have a blanket into prop impls for those. Note that I'm not against this QoL improvement, if we're adding it for string types, why not also add it anything that can be converted to VNode?

Good point xD I didn't even think about that!

tbh... I'm not sure how you achieve this though. Would that involve still doing the .into() at the end? I'm not super in favor of this as it doesn't improve much the usage... (html!(...) vs ....into())

So I tried this:

impl<T: Into<VNode>> IntoPropValue<VNode> for T {
    fn into_prop_value(self) -> VNode {
        self.into()
    }
}

But it fails because of conflicting implementation:

error[E0119]: conflicting implementations of trait `html::conversion::IntoPropValue<virtual_dom::vnode::VNode>` for type `virtual_dom::vnode::VNode`
   --> packages/yew/src/html/conversion.rs:183:1
    |
20  | impl<T> IntoPropValue<T> for T {
    | ------------------------------ first implementation here
...
183 | impl<T: Into<VNode>> IntoPropValue<VNode> for T {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `virtual_dom::vnode::VNode`

Did you have something else in mind?

[github-actions[bot]]

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

https://yew-rs-api--pr2872-cecton-pass-string-t-y3yv1z9w.web.app

_{(expires Thu, 06 Oct 2022 13:11:09 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

[github-actions[bot]]

Benchmark - SSR

Yew Master

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	445.605	459.604	451.094	3.927
Hello World	10	724.485	753.084	739.014	7.729
Function Router	10	2795.548	2851.397	2827.265	14.536
Concurrent Task	10	1009.647	1011.711	1010.755	0.685

Pull Request

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	394.506	408.317	399.609	4.998
Hello World	10	742.431	773.624	758.366	10.615
Function Router	10	2761.270	2821.949	2788.844	22.532
Concurrent Task	10	1009.450	1011.237	1010.405	0.652

[cecton]

(@hamza1311 I just added you to my fork in case you want to show me in the code directly)

[cecton]

I'm not sure why this ticket is in draft anymore... looks like it's complete but could be improve in some way. Will move it to ready to get more feedbacks.