Regression: elements are re-rendered even if they didn't change, when there's a list below them (worked in v0.19.3)

[andoalon]

Problem
Elements are re-rendered even if they didn't change, when there's a list below them. It works correctly in yew 0.19.3, but no longer in yew 0.20.0.

This is especially problematic when the re-rendered element is e.g. a <video> since when it gets re-rendered it stops and resets to the beginning. This is currently blocking me from updating 0.19.3 -> 0.20.0

Steps To Reproduce

1. Have a component that renders an element and a dynamic list below it
2. Change the list to cause a re-render of the component
3. The elements coming from the list get rendered, but so does the other element too (this can be easily seen in the chrome devtools since you can see the element flash when it changes)

Run/see the full repro in this repo: https://github.com/andoalon/yew-bug (just run trunk serve --open)

In short, it boils down to this:

#[function_component(Video)]
pub fn video() -> Html {
    let number = use_state_eq(|| None::<i64>);

    let time_update = {
        let number = number.clone();
        Callback::from(move |event| {
            // number_from_video() returns some number
            // based on the video's current position
            number.set(number_from_video(event));
        })
    };

    let number_vec = {
        if let Some(num) = *number {
            vec![html! {
                <button class="my-button">
                    {format!("Number: {num}")}
                </button>
            }]
        } else {
            Vec::new()
        }
    };

    let number_opt = number.map(|num| html! {
        <button class="my-button">
            {format!("Number: {num}")}
        </button>
    });

    html! {
        <>
            // The video element gets re-rendered when the list below changes
            //  in cases 2, 2.1, 3 and 3.1 (it does not in case 1). It also doesn't get
            // re-rendered in yew 0.19.3
            <video muted=true controls=true type={VIDEO_TYPE} src={VIDEO_URL}
                ontimeupdate={time_update}>
            </video>

            // Case 1: Works
            // if let Some(num) = *number {
            //     <button class="my-button">
            //         {format!("Number: {num}")}
            //     </button>
            // }

            { number_vec } // Case 2: Bug
            //{ for number_vec } // Case 2.1: Bug

            //{ number_opt } // Case 3: Bug
            //{ for number_opt } // Case 3.1: Bug
        </>
    }
}

Expected behavior
Elements are only re-rendered if they changed.

Observed behavior with <video>
It resets to the beginning and stops when the list below it changes. On a different note, I noticed that the muted attribute of <video> seems to not be working. Not sure if I misused it 🤔

Environment:

• Yew version: v0.20.0 (previously was using v0.19.3 where the issue was not present)
• Rust version: 1.69.0 (stable)
• Target, if relevant: wasm32-unknown-unknown (not sure if relevant)
• Build tool, if relevant: trunk (not sure if relevant)
• OS, if relevant: Windows 10 (not sure if relevant)
• Browser and version, if relevant: reproduced in Chrome 112.0.5615.138 (Official Build) (64-bit) and Firefox 112.0.2 (64-bit)

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

[WorldSEnder]

If you have a dynamically length list of children, and you want to make sure that the correct elements are "identified", use keyed lists. This is how you'd guarantee the previous behaviour, instead of relying on an implementation detail:

#[function_component(Video)]
pub fn video() -> Html {
    let number = use_state_eq(|| None::<i64>);

    let time_update = {
        let number = number.clone();
        Callback::from(move |event| {
            // number_from_video() returns some number
            // based on the video's current position
            number.set(number_from_video(event));
        })
    };

    let number_vec = {
        if let Some(num) = *number {
            vec![html! {
                <button key="button" class="my-button">
                    {format!("Number: {num}")}
                </button>
            }]
        } else {
            Vec::new()
        }
    };

    let number_opt = number.map(|num| html! {
        <button key="number" class="my-button">
            {format!("Number: {num}")}
        </button>
    });

    html! {
        <>
            // The video element gets reconciled with the existing video element and is not replaced
            <video key="video" muted=true controls=true type={VIDEO_TYPE} src={VIDEO_URL}
                ontimeupdate={time_update}>
            </video>

            // if let Some(num) = *number {
            //     <button key="case1" class="my-button">
            //         {format!("Number: {num}")}
            //     </button>
            // }

            { for number_vec } // Case 2: Works with keys

            { for number_opt } // Case 3: Works with keys
        </>
    }
}

[futursolo]

I believe this is actually a regression where the vectors are wrongly flattened by for into the parent VList.
This should be preventable by expanding Vectors to a separate VList.

    html! {
        <>
            // The video element gets reconciled with the existing video element and is not replaced
            <video muted=true controls=true type={VIDEO_TYPE} src={VIDEO_URL}
                ontimeupdate={time_update}>
            </video>

            // if let Some(num) = *number {
            //     <button class="my-button">
            //         {format!("Number: {num}")}
            //     </button>
            // }

            <> // Html macro should expand for expression into a fragment / VList
                { for number_vec } // Case 2: Works without keys
            </>

            <> // Html macro should expand for expression into a fragment / VList
                { for number_opt } // Case 3: Works without keys
            </>
        </>
    }

[andoalon]

I'm aware of keyed lists, but I tought they are only used to identify elements from the same list. I assumed whether or not using keys in number_vec would not affect what happens with the <video> tag which is outside (my bad).

It does fix the issue though (as long as all of the elements in the same fragment have keys (i.e. adding a key just to the <video> doesn't change the situation)) and also does wrapping the list in its own fragment as shown by futursolo, thank you both.

My understanding is that yew would diff each of the rendered elements from the component with the last rendered version to check whether an update was needed. Am I misunderstanding something here? Because the <video> tag definitely doesn't change 🤔

Or is futursolo right and it's indeed a bug? @WorldSEnder
If it's intended behavior I'm happy to close the issue

[WorldSEnder]

My understanding is that yew would diff each of the rendered elements from the component with the last rendered version to check whether an update was needed. Am I misunderstanding something here? Because the <video> tag definitely doesn't change thinking

While mostly correct, the point is that it's not clear which nodes should be diffed with which previous ones especially if the lists differ by length. As an aside and only considering the situation without keys, if both lists of children have the same length, nodes are currently diffed by position, i.e. the two lists are zipped and each pair reconciled. I don't think this part will change, even if it's fyi technically not guaranteed by the API. But you shouldn't be forced to use keys for layouts that don't have conditional children.

If list lengths differ, yew currently reserves the right to do "whatever it wants" when pairing up nodes from the two lists. Perhaps its best explained by example. Since nested children are not relevant, I'll omit those. In your original post the diffing would have to consider

<>                          <>
    <video />      ==>        <video />
                              <button />
                              <button />
</>                         </>

While my visual aligment is what you may expect, it's not so clear behind the scenes. What ends up happening is that the <video> from before gets reconciled with the <button> at the end of the list, and the two elements above are inserted "as new". This is implementation defined behaviour do not rely on it. Indeed, I guess it was different in yew 0.19. There is no a-priori choice that is correct, it might also reconcile with the other <button> or do what you intended it to. Looking at individual tag names or similar of list elements to figure out a "more correct" reconciliation would be overhead in the general case.

As an aside, Case 1 also spuriously works because of an implementation detail. It turns out that the if construct turns into an empty fragment if the condition is false. Both lists end up having the same length, so that would be

<>                          <>
    <video />      ==>        <video />
    <></>                     <button />
</>                         </>

This is similar to how futursolo's approach works. By nesting the conditionally rendered children in further fragments, they end up always consuming one spot in the list that contains the <video> element, thus (ab)using the special case outlined in the beginning.

Keyed lists solve the problem of not being able to identify children between renders. My solution would look like

<>                                      <>
    <video key="video" />      ==>        <video key="video" />
                                          <button key="button" />
                                          <button key="number" />
</>                                     </>

By using the special key attribute, it's made clear to yew which nodes to reconcile with which others, no matter the lists changing lengths. Node pairs with the same key from old and new vdom are identified and reconciled. That's also why the key must be unique in each list, but not globally, since it only matters in this step.

Relevant PR that touched it: #2330

Addendum: for the "flattening" of the following - at least I'm also not immediately sure why it works/doesn't work

            { number_vec } // Case 2: Bug
            { number_opt } // Case 3: Bug

I thought you'd have to use the for construction here, which indeed should flatten the elements into the list it is used in.

[futursolo]

I will take some time to explain in detail during the weekend, however my opinion is that I do not think for expression (or anything else) should make keys to be reconciled at a higher level hence make an originally keyless-safe part of a document fragment to be key-required.

I do wish to quickly point out the following:

As an aside, Case 1 also spuriously works because of an implementation detail.

In my opinion, conditional rendering should work without keys by design.
The current implementation clearly does this intentionally to reserve a slot for the if expression to prevent any unintentional shifting. Our docs do not use keys as well.

Background: In React, conditional rendering (e.g.: if is_loading { <p>"loading..."</p> }) is written as isLoading && <p>loading...</p>, I personally don't remember any React tutorial have told users that the p element should be keyed or else it won't work reliably. I do not see a reason why Yew should impose a key-ed requirement as well.

[andoalon]

Thank you for the detailed explanation, it's very useful. Intuitively though, I would assume yew that when I use { number_vec }, yew would treat that as its own list, and would try to reconcile it only with itself. In my (I guess naive) mental model it looks like this:

html! {
    <>
        <video />
        { number_vec }
        <a />
    </>
}

should turn into

<>
    <video />
    <></>
    <a />
</>

when number_vec is empty, and when it has some elements:

<>
    <video />
    <>
        <button />
        <button />
        <button />
    </>
    <a />
</>

so that

<>                  ==>            <>
    <video />  reconciles with =>    <video />
    <></>      reconciles with =>    <> <button /> <button /> <button /> </>
    <a />      reconciles with =>    <a />
</>                                </>

What I'm trying to say is that my intuition is that yew would know that I'm expanding a collection, and would take that into account when deciding what should get reconciled with what. Easier said than done, I guess 😅

Anyway, I have a question about keyed lists: at this moment I have no easy way to give unique keys to all of my dynamic elements. Is having a keyed list in which some elements have the same key a bad idea? In my current use-case it's enough that the <video> is identified correctly, so: is having a list with one unique key, but the rest being repeated, "valid"? Is that behavior defined? Something like this:

<video key="video" />
<button key="dont_care" />
<button key="dont_care" />
<button key="dont_care" />

[WorldSEnder]

Background: In React, conditional rendering (e.g.: if is_loading { <p>"loading..."</p> }) is written as isLoading && <p>loading...</p>, I personally don't remember any React tutorial have told users that the p element should be keyed or else it won't work reliably. I do not see a reason why Yew should impose a key-ed requirement as well.

React suffers from very much a similar issue, with a similar solution - although the reconciliation is slightly different and proceeds top-down. So you place a conditional element in the middle, which will make the video stop when you press the button, I believe. Hm, it seems it works as expected with the latest react version, although I distinctly remember having such a bug before with react 14 or so.

EDIT: You need to force it, and jsx syntax might differ here. You can definitely reproduce the problem in react if you want it, I just haven't found a way with jsx syntax.

import { useState } from "react";

export default function App() {
  let [a, setA] = useState(false);

  let elements = [
    <h1>Hello CodeSandbox</h1>,
    <button onClick={() => setA(true)}>Set a</button>
  ];
  if (a) {
    elements.push(<h2>Start editing to see some magic happen!</h2>);
  }
  elements.push(
    <video
      //key="video"
      src="https://sample-videos.com/video123/mp4/720/big_buck_bunny_720p_1mb.mp4"
    />
  );

  return <div className="App">{elements}</div>;
}

[WorldSEnder]

Is having a keyed list in which some elements have the same key a bad idea?

Yes. In "worst case" where you don't any identifying info, you could use the index in your list as a key via enumerate. The behaviour is currently not detected, but also not defined. I guess a debug-only check of lint would be nice to have to catch it.

[futursolo]

You need to force it, and jsx syntax might differ here.

A falsy value for a in React, renders nothing in the DOM, but still evaluate to false when you construct the React's children.
This always result in a slot being reserved even if the element is not rendered.
When you try to force it to happen with arrays manually, React will emit a warning to the developer console stating that keys are required in these situations.

The JSX expression is more equivalent to:

if (a) {
    elements.push(<h2>Start editing to see some magic happen!</h2>);
  } else {
    elements.push(false);
  }

That's why declarative syntax is safe without keys where arrays and iterators isn't as in a JSX expression / HTML macro, the declared block can always indicate there is an expression regardless of whether it is rendered.

[dylanowen]

I just ran into this and while it makes sense that yew reserves the right to do whatever it wants when pairing the nodes it feels unintuitive to patch child nodes in reverse order:

yew/packages/yew/src/dom_bundle/blist.rs

Line 172 in 2c4ea60

let mut lefts_it = lefts.into_iter().rev();

Especially for something like:

#[function_component]
fn BuggyBlist() -> Html {
    let state = use_state(|| false);

    let toggle = {
        let state = state.clone();
        Callback::from(move |_| state.set(!*state))
    };

    let header = html! {
        <>
            <input
                name="A"
                type="text"
            />
            <input
                name="B"
                type="text"
            />
        </>
    };

    if *state {
        html! {
            <>
                <button onclick={toggle}> {"Toggle"} </button>
                { header }
                // <></>
            </>
        }
    } else {
        html! {
            <>
                <button onclick={toggle}> {"Toggle"} </button>
                { header }
                <div>
                    { "end element" }
                </div>
            </>
        }
    }
}

We're adding and removing the end element but this manifests as re-rendering all of the previous input fields.

[futursolo]

Sorry about the delay. I finally have some time to sit down and explain my opinion on this issue.

In short, I do not think expanding {for expr} into the parent VList is a desirable behaviour.
In my opinion, all html! declared elements, components and expressions ({for expr} itself should be keyless-safe, but items expanded by expr isn't.) should be keyless-safe, which includes this issue.

My main concern of the current {for expr} behaviour, is that this can render previously keyless-safe VList / Children key-required,
by simply inserting {for expr} into somewhere of an html!, leading to confusing, hard to debug scenarios.

However, this is not the only concern around this extending behaviour. So I will discuss this in detail:

1. An extending for-expression is hard to spot, and hard to maintain.

Expanding {for expr} into the parent VList, renders all adjacent elements defined by html! key-required.

Whilst we can make a distinction between {contents} and {for contents}
I think due to limitations of Rust, we cannot make {contents} to implement for any type that is an iterator, the blank implementation is already implemented for std::fmt::Display.
This leaves user with an iterator content with no choice but to use {for contents} as that is more intuitive (and efficient?) to do than .collect::<Vec<_>>().

The above, which increased the likelihood of users using {for contents}, combined with {for expr}'s side effect could lead users into tedious debugging scenarios.

Suppose that a developer has created the following page:

html! {
    <div class="page-container gutter-left gutter-right">
        <main class="main">
            <header class="header header-stand gutter-bottom flex-container">
                <div class="header-container">
                    <h1 class="header-page-title h1-standout highlight gutter-bottom">{"Title"}</h1>
                    <nav>
                        <a href="/" class="nav-link current home nav-item-flex">
                            <span class="link-text">{"Home"}</span>
                        </a>
                        <a href="/" class="nav-link nav-item-flex">
                            <span class="link-text">{"Articles"}</span>
                        </a>
                        <a href="/" class="nav-link nav-item-flex">
                            <span class="link-text">{"Home"}</span>
                        </a>
                       <a href="/" class="nav-link nav-item-flex">
                            <span class="link-text">{"Log In"}</span>
                        </a>
                       <a href="/" class="nav-link nav-item-flex">
                            <span class="link-text">{"Register"}</span>
                        </a>
                    </nav>
                </div>
            </header>

            <div class="main-content max-width-xl">
                <div class="main-content-container gutter-top gutter-buttom">
                    {article_1}
                </div>
            </div>

            <div class="main-content max-width-xl">
                <div class="main-content-container gutter-top gutter-buttom">
                    {article_2}
                </div>
            </div>

            <div class="main-content max-width-xl">
                <div class="main-content-container gutter-top gutter-buttom">
                    {article_3}
                </div>
            </div>

            <div class="main-content max-width-xl">
                <div class="main-content-container gutter-top gutter-buttom">
                    {article_4}
                </div>
            </div>

            <div class="main-content max-width-xl">
                <div class="main-content-container gutter-top gutter-buttom">
                    {article_5}
                </div>
            </div>

            <form class="form-contact-us">
                <div class="form-field-container">
                    <input
                        name="email"
                        placeholder="E-Mail Address"
                        type="email"
                        class="content-field-input input-text-field gutter-top gutter-bottom styled-rounded"
                    />
                </div>
                <div class="form-field-container">
                    <input
                        name="phone"
                        placeholder="Phone Number"
                        type="text"
                        class="content-field-input input-text-field gutter-top gutter-bottom styled-rounded"
                    />
                </div>
                <div class="form-field-container">
                    <input
                        name="phone"
                        placeholder="Phone Number"
                        type="text"
                        class="content-field-input input-text-field gutter-top gutter-bottom styled-rounded"
                    />
                </div>
                <div class="form-field-container full-width">
                    <textarea
                        name="message"
                        placeholder="Message"
                        class="content-field-input input-textarea gutter-top gutter-bottom styled-rounded"
                    ></textarea>
                </div>
                {for random_3_questions}

                <div class="form-field-container">
                    <input
                        type="submit"
                        value="Submit"
                        class="content-field-input button gutter-top styled-rounded"
                    />
                </div>
            </form>

            {for seasonal_contents}

            <div class="footer-paginator">
                <div class="paginator-arrow paginator-prev">
                    <i class="arrow-left"></i>
                    <span>{"Previous Page"}</span>
                </div>
                {paginator_nos}
                <div class="paginator-arrow paginator-next">
                    <i class="arrow-right"></i>
                    <span>{"Next Page"}</span>
                </div>
            </div>

            <footer class="page-footer">
                <div class="footer-container">
                    <div class="footer-line">
                        <h5>
                            <span class="font-secondary color-secondary">{"©"}</span>
                            <span class="font-primary color-secondary">{"My Website"}</span>
                        </h5>
                    </div>
                </div>
            </footer>
        </main>
        <aside class="page-sidebar gutter-right styled-rounded">
            {sidebar_contents}
        </aside>
    </div>
}

By including {for seasonal_contents} and {for random_3_questions}, all child nodes of <main /> and <form /> are now key-required.

The developer of this page, is finding that the contents are not updating as expected that contents are getting resetted from time to time, and is trying to debug this page.

This creates some questions for the debugging process:

1. How long would the developer take to realise that {for expr} is the issue here and it is rendering its siblings as key-required?
2. Is it easy to figure out the elements that should be keyed and fix them?
3. Would it be easy for the developers to keep track of elements when being placed under which element will require keys
   as they extend this page?
4. Half a year later, the developer comes back to add some new features, what is the likelihood that they still remember Q3?

In addition, this is not only a concern of when one is working on their own code, but also when reviewing other people's code when adding a {for expr} looks like a small change
so the reviewer usually do not expand the code and think it very deeply before moving on to other parts of the PR.

2. What key should a VNode use?

If we assume that the first point isn't an issue, this developer is willing to deal with it,
expanding into the same VList still has another issue that can very easily forfeit the user's effort of trying to key elements.

Let's assume that we have a component named Parent:

#[function_component]
fn Parent(props: &ChildrenProps) -> Html {
    html! {
        // Let's assume that GrandParent
        // process Children by manipulation so we cannot use any `<></>` isolation tricks.
        <GrandParent>
            <header key="header">{"Parent Header"}</header>
            {for props.children.iter()}
        </GrandParent>
    }
}

The author knows that if an for-expression is used, they need to key all the sibling elements as well.
So they keyed the header element.
As it is fully keyed, Children of the Parent component should reconcile as expected, given we have a way to enforce Children to be fully keyed.
The author then published the Parent component as a crate to crates.io.

A couple days later, a user created an App component using Parent.
The user also knows that they have to key all elements if an for-expression is used so they keyed their app header.

#[function_component]
fn App() -> Html {
    let contents: Vec<Html> = create_contents();

    html! {
        <Parent>
            <div key="header">{"My App Header"}</div>
            {for contents}
        </Parent>
    }
}

Despite both sides have taken precaution to make sure that all items are keyed, the application still doesn't work properly.
The effort of both sides are nullified because they both used header as the key.

In this case, would it be reasonable to assume that it's the responsibility of the author of Parent to exhaustively list the keys they are using and the creator of App should read the fine manual of the parent component?

3. It's very unintuitive to key certain expressions.

If a VList not fully-keyed, the behaviour is equivalent to unkeyed. So it's important to key every child node in a VList.
By adding {for expr}, all siblings are required to be keyed.

However, certain expressions are very unintuitive to key.
This is OK as currently as I would assume that they are never designed to be keyed (nor needed if used in html!).

For the following html!,

html! {
    <Parent>
        if show_header {
            <div>
                {"My App Header"}
            </div>
        }   
        {for contents}
    </Parent>
}

It might be easy to assume that it should be keyed as following:

html! {
    <Parent>
        if show_header {
            <div key="header">
                {"My App Header"}
            </div>
        }   
        {for contents}
    </Parent>
}

However, this does not cover the default value of this expression as it is VNode::default(), so it actually needs to be keyed in the following way by adding a fragment:

html! {
    <Parent>
        <key="header">
            if show_header {
                <div>
                    {"My App Header"}
                </div>
            }
        </>
        {for contents}
    </Parent>
}

The extreme (yet common) scenario would be that we need to key text nodes.

html! {
    <Parent>
        <key="header">
            {"My App Header"}
        </>
         <key="header2">
            {"My App Header 2"}
        </>
        {for contents}
    </Parent>
}

I think this is not only hard to justify, but also beyond the effort that most people is willing to spend on a feature.

4. Conclusion

In my opinion, keys should only be required in the following scenarios:

1. Inside of html created by Vec / Iterator.

   Components' keyless-safety should not be impacted by anything else declared in html!.
   (Or else we should make a unsafe {} clause for expressions that could impact it this way.)

2. When a component needs to be forcibly replaced,

   i.e.: a different key for the same tag / component forces a destroy and create cycle, instead of reusing the existing component / tag.

I think the behaviour described by @dylanowen is also more desirable, which we should consider.

I personally think this limits scenarios where keys are required, makes key requirement easier to remember and is more difficult to get confused of when do you need to use it.

The reality is, even with React's easier key requirement and warnings in developer console,
it is still not uncommon to see React code that misuse / forget keys.

I certainly wish that we do not make the key requirement more complicated and subtle (1), making coming up keys itself more difficult (2), or make users life harder (3).

In addition, the only reason why {for expr} should be expanded to current VList I can come up with is because we want to manipulate Children in which itself doesn't work properly for reasons I described in #3256.

[dylanowen]

I was also thinking about the ergonomics of this a bit. I always thought (before reading the source) that it would be possible to partially key child elements. Being able to just key the important elements without keying everything would make it easier to spot check that the essential elements will always be reused regardless of surrounding changes or surrounding lists. Is there a reason this isn't supported? It seems like the keyed and unkeyed implementations are just less generic partially keyed implementations.