Skip to content

Omit conditional elements without an else branch when present in children #3274

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

Conversation

texodus
Copy link

@texodus texodus commented May 19, 2023

Description

This PR proposes a method to address the behavior described in #3256. It only changes the behavior of conditional syntax without an else block.

To overcome the nature of the abstraction between HtmlChildrenTree and HtmlIf, I've added a newtype struct HtmlIfIterItem which is used to generate tokens for HtmlIf in this context. There is also judicious use of safe-by-inspection .unwrap() for calls to . to_node_iterator_stream() to avoid code duplication for this trait which returns Option<_>. There are likely simpler ways to do both of these things, comments welcome.

I've added a test to assert/illustrate the behavior in question.

Fixes #3256

Checklist

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

@texodus texodus force-pushed the html-omit-conditionals-from-children branch from 55d5d08 to 8a26386 Compare May 19, 2023 04:34
@github-actions
Copy link

github-actions bot commented May 19, 2023

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

https://yew-rs-api--pr3274-html-omit-conditiona-9cjbipiu.web.app

(expires Fri, 26 May 2023 16:57:33 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented May 19, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 104.303 106.088 +1.785 +1.712%
boids 175.120 178.646 +3.526 +2.014%
communication_child_to_parent 96.931 102.454 +5.523 +5.698%
communication_grandchild_with_grandparent 109.133 116.412 +7.279 +6.670%
communication_grandparent_to_grandchild 105.312 111.848 +6.536 +6.206%
communication_parent_to_child 93.981 99.052 +5.070 +5.395%
contexts 109.846 111.282 +1.437 +1.308%
counter 90.563 93.066 +2.503 +2.764%
counter_functional 90.958 92.795 +1.837 +2.020%
dyn_create_destroy_apps 93.319 96.255 +2.936 +3.146%
file_upload 104.657 110.052 +5.395 +5.154%
function_memory_game 169.771 175.453 +5.682 +3.347%
function_router 339.161 378.965 +39.804 +11.736%
function_todomvc 164.188 170.576 +6.388 +3.890%
futures 228.079 229.732 +1.653 +0.725%
game_of_life 110.745 117.214 +6.469 +5.841%
immutable 188.867 198.327 +9.460 +5.009%
inner_html 87.109 87.109 0 0.000%
js_callback 113.721 116.837 +3.116 +2.740%
keyed_list 202.176 216.446 +14.271 +7.058%
mount_point 90.237 91.184 +0.946 +1.049%
nested_list 114.743 117.280 +2.537 +2.211%
node_refs 97.723 104.634 +6.911 +7.072%
password_strength 1588.771 1592.340 +3.569 +0.225%
portals 99.020 103.203 +4.184 +4.225%
router 310.121 349.384 +39.263 +12.660%
simple_ssr 145.285 146.135 +0.850 +0.585%
ssr_router 377.176 416.957 +39.781 +10.547%
suspense 111.411 115.362 +3.951 +3.546%
timer 93.336 96.317 +2.981 +3.194%
timer_functional 101.644 104.607 +2.964 +2.916%
todomvc 145.481 151.394 +5.912 +4.064%
two_apps 91.157 93.726 +2.568 +2.818%
web_worker_fib 155.475 159.617 +4.143 +2.664%
webgl 89.666 90.652 +0.986 +1.100%

⚠️ The following examples have changed their size significantly:

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 104.303 106.088 +1.785 +1.712%
boids 175.120 178.646 +3.526 +2.014%
communication_child_to_parent 96.931 102.454 +5.523 +5.698%
communication_grandchild_with_grandparent 109.133 116.412 +7.279 +6.670%
communication_grandparent_to_grandchild 105.312 111.848 +6.536 +6.206%
communication_parent_to_child 93.981 99.052 +5.070 +5.395%
contexts 109.846 111.282 +1.437 +1.308%
counter 90.563 93.066 +2.503 +2.764%
counter_functional 90.958 92.795 +1.837 +2.020%
dyn_create_destroy_apps 93.319 96.255 +2.936 +3.146%
file_upload 104.657 110.052 +5.395 +5.154%
function_memory_game 169.771 175.453 +5.682 +3.347%
function_router 339.161 378.965 +39.804 +11.736%
function_todomvc 164.188 170.576 +6.388 +3.890%
game_of_life 110.745 117.214 +6.469 +5.841%
immutable 188.867 198.327 +9.460 +5.009%
js_callback 113.721 116.837 +3.116 +2.740%
keyed_list 202.176 216.446 +14.271 +7.058%
mount_point 90.237 91.184 +0.946 +1.049%
nested_list 114.743 117.280 +2.537 +2.211%
node_refs 97.723 104.634 +6.911 +7.072%
portals 99.020 103.203 +4.184 +4.225%
router 310.121 349.384 +39.263 +12.660%
ssr_router 377.176 416.957 +39.781 +10.547%
suspense 111.411 115.362 +3.951 +3.546%
timer 93.336 96.317 +2.981 +3.194%
timer_functional 101.644 104.607 +2.964 +2.916%
todomvc 145.481 151.394 +5.912 +4.064%
two_apps 91.157 93.726 +2.568 +2.818%
web_worker_fib 155.475 159.617 +4.143 +2.664%
webgl 89.666 90.652 +0.986 +1.100%

@github-actions
Copy link

github-actions bot commented May 19, 2023

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 431.703 497.188 465.631 20.388
Hello World 10 727.071 804.278 763.552 29.867
Function Router 10 2684.702 2970.857 2848.961 102.622
Concurrent Task 10 1008.962 1011.445 1010.539 0.819
Many Providers 10 1907.286 2134.412 2040.879 70.266

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 427.949 493.047 455.465 20.768
Hello World 10 748.771 900.349 818.043 45.086
Function Router 10 2794.486 3074.313 2927.916 78.800
Concurrent Task 10 1009.075 1011.793 1010.259 0.921
Many Providers 10 2009.526 2183.238 2096.476 59.786

@texodus texodus force-pushed the html-omit-conditionals-from-children branch 5 times, most recently from b3c16a4 to fb33158 Compare May 19, 2023 06:10
@texodus texodus force-pushed the html-omit-conditionals-from-children branch from fb33158 to 97eecad Compare May 19, 2023 16:54
@futursolo
Copy link
Member

futursolo commented May 20, 2023

Thank you for the pull request!

However, after examining the original issue, I believe the current behaviour is intentional and correct.

Here are the reasons why I think the current behaviour is correct:

Rust is primarily an expression language

Quoted from Rust Reference Book:

This means that most forms of value-producing or effect-causing evaluation are directed by the uniform syntax category of expressions.

In other words, this means that almost every piece of code that actually does something in Rust are most likely expressions.

This design choice dictates several iconic language features in Rust. The main feature that is relevant in this case is expressions always produce a value.

E.g.: if b {} is an expression and let a = if b {}; is a valid statement. () is assigned to a.

At a glance, it might be intuitive to assume that the following code

let h = html! {
    <>
        if b {
            <span>{ "C" }</span>
        }
    </>
};

is roughly a syntax sugar of:

let h = {
    let mut nodes = VList::new();

    if b {
        // html! actually uses an iterator to construct children.
        // This example is trying to simplify what it does in this case.
        nodes.push(html! {<span>{"b"}</span>});
    }

    VNode::from(nodes)
};

However, if we take a look at the last expression of the if block, we can find that the block has a return type of Html instead of (). This means that it is an expression and not statement as "The type of ExpressionWithBlock expressions when used as statements must be the unit type.". As expressions always produce a value, it should desugar to:

let h = {
    let mut nodes = VList::new();

    nodes.push(
        if b {
            html! {<span>{"b"}</span>}
        }
    );

    VNode::from(nodes)
};

The VList here is not relevant, so we can further simplify this to:

let h = if b {
    html! { <span>{ "C" }</span> }
};

If you try to compile the above code, Rust compiler will generate a compiler diagnostic stating that this is invalid because the else branch is missing and the complier expects the default value of this block expression to be returned (()).

image

This is not difficult to understand, as blocks are expressions, they need to evaluate to a deterministic value type.
With block's else omitted, all existing conditional blocks must return the default value of ().

In this case, for if b { html! { <span>{ "C" }</span> } }, both Html and () are possible, which confuses the compiler.

To satisfy the compiler, one would need to write:

let h = if b {
    html! { <span>{ "C" }</span> }
} else {
    Html::default()
};

The next question becomes: how do we interpret if expressions in html!?

To solve this situation in html!, I can come up with 2 solutions:

  1. Require an else block in html!.
  2. Make default value of block expressions inside html! VNode::default().

The first solution is to make sure that every time an if expression is used, an else block is required. I.e.:

html! {
    <>
        if b {
            <span>{ "C" }</span>
        } else {
            <></>
        }
    </>
}

I think it is not difficult to reach a conclusion that this is cumbersome to write and in many cases, an unnecessary burden imposed on Yew users.

For the second solution, it should now make sense, that we make block expressions to evaluate to VNode as their default value if an else block is missing.
(The implementation is a little bit different and would allow any type that is accepted by VNode::from. But the above rationale should still stand.)

<></> is actually useful and important in some situations

Consider the following example (borrowed from @WorldSEnder from #3256, adapted for Yew):

use yew::prelude::*;

#[function_component]
fn App() -> Html {
    let a = use_state_eq(|| true);
    // hooks syntax is different in the master branch.
    // dependency should be placed as the first argument when testing with this pr.
    let on_set_a = use_callback(
        |_v, input| {
            input.set(!**input);
        },
        a.clone(),
    );

    html! {
        <div>
            <h1>{"Hello CodeSandbox"}</h1>
            <video width="720" src="https://sample-videos.com/video123/mp4/720/big_buck_bunny_720p_1mb.mp4" controls={true} />
            <br />
            if *a {
                <>
                    <h2>{"Click the button below for some magic!"}</h2>
                    <br />
                </>
            }
            <button onclick={on_set_a}>{"Toggle a"}</button>
        </div>
    }
}

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

In Yew v0.20.0, clicking on the button will not interfere with video playing. However, after applying this pull request, the video will be interrupted when clicking on the button.

When a is false, it evaluates to <></>. This protects the structural integrity of a VList when it is constructed by html!. After applying this pull request, the video will be interrupted and will require a key to protect the video element.

Conclusion

In conclusion, my personal opinion is that the current behaviour is more favourable so I am not in favour of upstreaming this pull request as a fix to #3256.

@texodus
Copy link
Author

texodus commented May 21, 2023

Appreciate the detailed response @futursolo. You've convinced me this isn't the right fix. I will move my feedback to #3256

@texodus texodus closed this May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conditional Rendering and Children component interact strangely
2 participants