Allow using Option<VNode> in html!() macro #2792

cecton · 2022-07-27T16:51:49Z

Description

Quality of life improvement that allows using Option<html! {}> inside html! {} macros.

Example:

fn view() {
    let inner = some_condition.then(|| {
        html! {
            <div>{"Some conditional stuff"}</div>
        }
    });

    html! {
        <>
        <div>{"Permanent stuff"}</div>
        {inner}
        </>
    }
}

Checklist

I have reviewed my own code
I have added tests

github-actions · 2022-07-27T16:56:29Z

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

https://yew-rs-api--pr2792-cecton-option-vnode-1xcjpour.web.app

_{(expires Tue, 20 Sep 2022 20:27:50 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

github-actions · 2022-07-27T17:05:06Z

Size Comparison

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
boids	170.251	170.258	+0.007	+0.004%
communication_child_to_parent	90.461	90.460	-0.001	-0.001%
communication_grandchild_with_grandparent	105.415	105.422	+0.007	+0.006%
communication_grandparent_to_grandchild	101.304	101.313	+0.010	+0.010%
communication_parent_to_child	87.597	87.596	-0.001	-0.001%
contexts	107.845	107.843	-0.002	-0.002%
counter	85.497	85.497	0	0.000%
counter_functional	86.025	86.026	+0.001	+0.001%
dyn_create_destroy_apps	88.369	88.367	-0.002	-0.002%
file_upload	100.079	100.075	-0.004	-0.004%
function_memory_game	163.802	163.795	-0.007	-0.004%
function_router	348.551	348.547	-0.004	-0.001%
function_todomvc	158.604	158.607	+0.004	+0.002%
futures	221.969	221.970	+0.001	+0.000%
game_of_life	105.760	105.762	+0.002	+0.002%
immutable	181.582	181.582	0	0.000%
inner_html	82.362	82.364	+0.002	+0.002%
js_callback	111.374	111.375	+0.001	+0.001%
keyed_list	195.558	195.551	-0.007	-0.003%
mount_point	85.115	85.114	-0.001	-0.001%
nested_list	112.592	112.586	-0.006	-0.005%
node_refs	92.980	92.981	+0.001	+0.001%
password_strength	1548.167	1548.168	+0.001	+0.000%
portals	96.280	96.277	-0.003	-0.003%
router	318.202	318.188	-0.015	-0.005%
simple_ssr	152.167	152.164	-0.003	-0.002%
ssr_router	394.007	394.032	+0.025	+0.006%
suspense	109.156	109.155	-0.001	-0.001%
timer	88.339	88.342	+0.003	+0.003%
todomvc	139.731	139.733	+0.002	+0.001%
two_apps	86.109	86.109	0	0.000%
web_worker_fib	152.241	152.247	+0.006	+0.004%
webgl	84.830	84.828	-0.002	-0.002%

✅ None of the examples has changed their size significantly.

WorldSEnder · 2022-07-27T19:35:20Z

If you want to follow the paths of Vec, then I think what's needed is an impl similar to

yew/packages/yew/src/utils/mod.rs

Line 26 in c28a71e

impl<IN: Into<OUT>, OUT> From<Vec<IN>> for NodeSeq<IN, OUT> {

impl<IN: Into<OUT>, OUT> From<Option<IN>> for NodeSeq<IN, OUT>

cecton · 2022-07-28T07:03:32Z

Thanks @WorldSEnder !!

.... 🤔 looks like type erasure or something..... tbh I'm not sure to understand the sorcery but I'm open for a better option than what I'm doing now (enumerating the types that are "string").

(Though what I'm doing doesn't have huge drawbacks imo... worst case scenario, someone would need to make .to_string() themselves to get the thing work as before. It sounds quite reasonable.)

WorldSEnder · 2022-07-28T13:12:10Z

It sounds quite reasonable

I actually think it would strain some users.

.... thinking looks like type erasure or something..... tbh I'm not sure to understand the sorcery

Maybe the following playground can help https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4dbe080258bd7da4175566d886e1899c. Try to remove the bounds on impl<T: _> in the impls there to, too!

Each child expression of a html node (element or component) is converted via Into::<NodeSeq<_, _>>::into( #expr ). Two parameters are to deduce: IN and OUT. OUT is determined by the type of children declared on the parent, most often OUT = VNode from ChildrenRenderer. IN is deduced by trait impl. Note that, like in the above playground, there must only be one matching impl, otherwise the above is ambiguous. This is why the IN: Into<OUT> bound on these impls is important, so rust can see that there is no impl Vec<IN>: Into<OUT> for example, and only the vector - not the blanket - impl matches. Since there is was no impl From<Option<VNode>> for VNode before the PR, the proposed trait impl could work, as far as I can tell.

ranile · 2022-07-28T17:56:53Z

packages/yew/src/virtual_dom/vnode.rs

+    u8, u16, u32, u64, u128,
+    i8, i16, i32, i64, i128,
+    f32, f64,
+    bool,
+    usize, isize,


This adds something that #1637 tried to get rid of (for props). We don't want to be implicitly allocating Strings.

With this change, if someone were to pass in a number, it would automatically get converted to String, incurring a heap allocation. That should be explicit, just like it is props

I'm not sure I'm following... before when you passed a primitive like that into the html!{} macro it would call ToString::to_string on it 🤔 am I missing something?

Alright sorry I misread what you said!!

Ok so I agree with you and I'm all in favor of removing those! If I do so I will need to update a lot of tests (I can do it, np).

In the current implementation of VNode, ToString is used and string are allocated implicitly. But personally I do think it's best to be explicit. Especially when it involves allocating strings on the heap like that.

(Though this idea is conflicting with what @WorldSEnder point of view)

With this change, if someone were to pass in a number, it would automatically get converted to String, incurring a heap allocation. That should be explicit, just like it is props

(so it's already how it works actually. I did not introduce that. I added this for compatibility because the tests were failing)

To clarify my viewpoint: The current behavior is to convert via T: ToString which has a blanket impl<T: Display> ToString for T. Arguably, the String will get converted to a JsString soon after. That second allocation can not be avoided. I wonder if it was possible to use the Display impl to go directly to the JsString but that allocation (being part of FFI) is neither surprising nor avoidable. That being said, if a user would have to write .to_string() manually, that would lead to (potentially) double the work, actually, since the resulting String would still need to be converted to a JsString as part of FFI.

As an aside, I think it's worthwhile to at least support fmt::Arguments (which is currently automatic through Display -> ToString).

Tl;dr: Custom logic that avoids the current String allocation welcome, but pushing it down to the user will only result in more allocations.

I have an(other) idea! 💡

What if we make our own syntax for string formatting. Maybe something like this:

<div>format!(....)</div>

(notice the lack of {} around format! This is because it is not the actual format macro.

With that, instead of storing strings directly, we can store: the formatting string (static str) + the arguments (must implement ImplicitClone).

When the node is rendered, we compare all the arguments for equality.

This would avoid string heap allocation.

@WorldSEnder @futursolo @hamza1311 I think I can make an implementation for this syntax:

fn compile_pass() { ::yew::html! { <div>format!("Hello {}!", "World")</div> }; }

It converts format!(...) to a VFormat type that holds all the arguments ("Hello {}!" and "World") and does a PartialEq between those arguments and the ones from another VFormat.

// https://users.rust-lang.org/t/partialeq-eq-hash-any/70495 // https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c6be9c99223bb83fefc3094d42724f8b #[derive(Clone)] pub struct VFormat { arguments: Rc<Vec<Box<TypeTagged<dyn TaggedEq>>>>, } impl PartialEq for VFormat { fn eq(&self, other: &VFormat) -> bool { self.arguments == other.arguments } }

However, it's rather complicated... if you don't mind I would prefer to do this in a separate PR and get this one merged. If this PR has no impact on performance at the moment, I think it's safe to merge. Let me know what you think.

I think format! and html! are very similar which both creates an owned instance (String vs VNode) based on formatting a certain representation with variables and can involve allocations if needed.

I am in favour of merging current implementation of From<_> for VText.

arguments: Rc<Vec<Box<TypeTagged<dyn TaggedEq>>>>,

This means that for each VFormat, we have an allocation overhead of:

Rc<_>.

Vec<_> for each argument.

Box<_> for each argument.

I feel it might be faster to use std::format! as it involves a single allocation overhead of creating a String.

In addition, types that implements std::fmt::Display are not required to implement PartialEq (nor does PartialEq sound for certain types like f64).

Very good point... I think the VFormat thingy will reduce string formatting and conversion to JS but will increase the number of allocations. I'm not sure it's worth it.

@futursolo I think I can reduce this to a single box for the entirety of the arguments. (But that won't solve the issue where PartiallEq is unsound (like for floats).

cecton · 2022-07-28T18:33:03Z

I actually think it would strain some users.

nahh. In Rust you always have to call .to_string() to convert stuff to strings. It's just normal stuff. In the html!{} macro we expect the user to either put HTML components or strings. If you provide an arbitrary object, it's up to you to make Yew understand how to render it.

Let me know your thought

github-actions · 2022-08-17T07:20:46Z

Benchmark - SSR

Yew Master

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	345.576	347.830	346.276	0.741
Hello World	10	614.453	616.598	615.935	0.566
Function Router	10	2326.340	2339.948	2331.792	4.982
Concurrent Task	10	1007.937	1009.545	1008.765	0.527

Pull Request

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	307.299	308.559	307.767	0.370
Hello World	10	593.823	595.158	594.433	0.486
Function Router	10	2230.094	2241.118	2234.955	3.477
Concurrent Task	10	1008.319	1009.724	1009.020	0.477

cecton · 2022-09-12T05:09:42Z

I think this PR is ready so please let me know if there is anything I need to fix.

cecton · 2022-09-12T06:19:04Z

I think the point of @WorldSEnder still stand:

Custom logic that avoids the current String allocation welcome, but pushing it down to the user will only result in more allocations.

If we think that this PR will degrade the current status I really don't mind scrapping it, no worry.

WorldSEnder · 2022-09-12T08:29:39Z

I still think the original goal of just allowing Option<VNode> to be used in html! is worth. If you aren't so sure how to achieve that without also changing the blanket impl<T: ToString> From<T> for VNode instance, I think I can come up with a minimal PR for that, borrowing your test case ;)

cecton · 2022-09-12T10:55:15Z

@WorldSEnder that would be great!

I did try what you proposed in here #2792 (comment) but I couldn't make it work... feel free to use the branch and keep the PR if you want! I will add you to the fork

WorldSEnder · 2022-09-13T20:30:23Z

Alright, got it to work. Can someone else review this, I don't want to "approve" my own code :)

cecton

Wow it was pretty simple! I don't know what blocked me last time... Thanks though!

(I can't approve as I am the author of the PR)

cecton · 2022-09-14T07:47:29Z

packages/yew/src/utils/mod.rs

+impl<IN: Into<OUT>, OUT> From<Option<IN>> for NodeSeq<IN, OUT> {
+    fn from(val: Option<IN>) -> Self {
+        Self(
+            val.map(|s| vec![s.into()]).unwrap_or_default(),


I think you can actually iterate over options:

Suggested change

val.map(|s| vec![s.into()]).unwrap_or_default(),

val.into_iter().collect(),

I tested this on godbolt.org, and the two versions produce different assembly, where the former is slightly smaller (llvm seems to optimize the case that val = None differently) than using collect, so I stuck with that one.

Good catch though, do you think I should add a comment explaining that?

I don't know 😅 my experience is more in high level languages...

Generally the rule is: if the intention is not obvious in the code, then it should be documented. So I would tend to say yes.

Long story:

Personally I wouldn't optimize that way because of my lack of experience. I just can't reason with it, I have too many questions in my mind. For example, is the code in assembly better for x86 or for WASM? Could it be different? Will that change in future releases of Rust/LLVM? I'm not asking you like they are relevant questions, I really don't know if those questions even make sense at all. I'm not even sure how to measure/evaluate which assembly code is faster (in terms of cycles or something I guess). So yeah I'm a total noob sorry xD I just can't answer for sure.

But I think that's the good part of the Rust community, we come with very different backgrounds. I prefer the readability of the iterator version but I totally agree it's more important to have fast code. The only reason for me to be against faster code is if it's very difficult to maintain (like if you would do everything unsafe (extreme scenario)). But here it's simple in both cases, the later code is more elegant which is a weak advantage compared to speed and size.

cecton · 2022-09-14T12:57:34Z

@hamza1311 @futursolo do you want to approve this one? Seems trivial thanks to @WorldSEnder

cecton changed the title ~~Add test to allow using Option<VNode> in html!() macro~~ Allow using Option<VNode> in html!() macro Jul 28, 2022

This was referenced Jul 28, 2022

Impl From<&String> for IString yewstack/implicit-clone#8

Closed

Impl ImplicitClone on bool, usize and isize yewstack/implicit-clone#9

Merged

ranile reviewed Jul 28, 2022

View reviewed changes

cecton marked this pull request as ready for review August 20, 2022 10:12

implement html from Option<VNode>

1dabd9c

WorldSEnder force-pushed the cecton-option-vnode branch from 31b161e to 1dabd9c Compare September 13, 2022 20:22

github-actions bot approved these changes Sep 13, 2022

View reviewed changes

cecton commented Sep 14, 2022

View reviewed changes

futursolo approved these changes Sep 14, 2022

View reviewed changes

ranile approved these changes Sep 14, 2022

View reviewed changes

ranile merged commit 81f7ea4 into yewstack:master Sep 14, 2022

cecton deleted the cecton-option-vnode branch September 29, 2022 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using Option<VNode> in html!() macro #2792

Allow using Option<VNode> in html!() macro #2792

cecton commented Jul 27, 2022 •

edited

Loading

github-actions bot commented Jul 27, 2022 •

edited

Loading

github-actions bot commented Jul 27, 2022 •

edited

Loading

WorldSEnder commented Jul 27, 2022

cecton commented Jul 28, 2022

WorldSEnder commented Jul 28, 2022 •

edited

Loading

ranile Jul 28, 2022

cecton Jul 28, 2022

cecton Jul 28, 2022

cecton Jul 28, 2022

WorldSEnder Jul 28, 2022 •

edited

Loading

cecton Jul 30, 2022

cecton Aug 17, 2022

futursolo Aug 17, 2022 •

edited

Loading

cecton Aug 17, 2022

cecton Aug 19, 2022

cecton commented Jul 28, 2022

github-actions bot commented Aug 17, 2022 •

edited

Loading

cecton commented Sep 12, 2022

cecton commented Sep 12, 2022

WorldSEnder commented Sep 12, 2022

cecton commented Sep 12, 2022

WorldSEnder commented Sep 13, 2022

cecton left a comment

cecton Sep 14, 2022

WorldSEnder Sep 14, 2022 •

edited

Loading

cecton Sep 14, 2022

cecton commented Sep 14, 2022

	val.map(\|s\| vec![s.into()]).unwrap_or_default(),
	val.into_iter().collect(),

Allow using Option<VNode> in html!() macro #2792

Allow using Option<VNode> in html!() macro #2792

Conversation

cecton commented Jul 27, 2022 • edited Loading

Description

Checklist

github-actions bot commented Jul 27, 2022 • edited Loading

github-actions bot commented Jul 27, 2022 • edited Loading

Size Comparison

WorldSEnder commented Jul 27, 2022

cecton commented Jul 28, 2022

WorldSEnder commented Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WorldSEnder Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

futursolo Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Jul 28, 2022

github-actions bot commented Aug 17, 2022 • edited Loading

Benchmark - SSR

Yew Master

Pull Request

cecton commented Sep 12, 2022

cecton commented Sep 12, 2022

WorldSEnder commented Sep 12, 2022

cecton commented Sep 12, 2022

WorldSEnder commented Sep 13, 2022

cecton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WorldSEnder Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Sep 14, 2022

cecton commented Jul 27, 2022 •

edited

Loading

github-actions bot commented Jul 27, 2022 •

edited

Loading

github-actions bot commented Jul 27, 2022 •

edited

Loading

WorldSEnder commented Jul 28, 2022 •

edited

Loading

WorldSEnder Jul 28, 2022 •

edited

Loading

futursolo Aug 17, 2022 •

edited

Loading

github-actions bot commented Aug 17, 2022 •

edited

Loading

WorldSEnder Sep 14, 2022 •

edited

Loading