Issue with unkeyed VDOM list items

[sirrodgepodge]

Description

First, I'm new to Rust and a project in Yew has been my path to learning since I know JS front development pretty well already, super grateful that this project exists, thank you!

On to the question, in React there's a concept of the "key" attribute for items in lists which allow a user to add a unique ID to list items.

This ensures that nodes in lists can be properly tracked, since order alone is not enough. I see no equivalent here in Yew and sure enough if I remove a list item from the middle of the list, the DOM node at the end of the list is removed and the content from the subsequent list nodes all gets shifted up into the node one above them (as opposed to the node that I actually removed getting destroyed and the subsequent content all staying in the same place).

Expected Results

When item in the middle of the list is removed, remove the corresponding DOM node, don't alter subsequent DOM nodes in list.

<div
    class="__game_container",
>
    { for state.game_objects.iter().map(|k| {
      html! {
                    <RenderObj:
                        transition_on= render_obj.transition_on,
                        direction= render_obj.direction,
                        coords= render_obj.coords,
                    />
      }
    }) 
</div>

Actual Results

When VDOM node from the middle of the list is removed, in the actual DOM a node is removed from the end of the list and all the content that was in nodes following the removed VDOM node gets shifted up by one.

Context (Environment)

• Rust: v1.35.0-nightly
• yew: v0.7.0 (just switched to latest commit: 150e5af)
• target: wasm32-unknown-unknown
• cargo-web: v0.6.23
• browser: Brave

[jstarry]

@sirrodgepodge this would be a great feature to have! Would you like to take stab at implementing it?

[sirrodgepodge]

thank you @jstarry, would absolutely love to, pretty slammed in the day job now so don't want to promise anything, if no one else does it someday I will though :)

[jeremyscatigna]

Would love to invest time on this 🙂 @jstarry would love to get some guidance if possible

[jstarry]

@jeremyscatigna Great, this is a good task to get your feet wet. You're mostly going to be digging into 2 areas of the code base. The macro and virtual dom diffing parts.

I think we only need keyed elements inside "list" nodes. The virtual dom list node code is here. You'll need to make changes to VDiff::apply so that instead of assuming elements are in order, we search element keys to find matches.

As for keyed elements, I think any element could be keyed. So maybe we need a trait for KeyedElement so that we can check keys (if set).

The macro code is split up for components vs tags..

Component:

html! {
   <MyComponent key={1} />
}

Macro code for parsing component properties (we already have special handling for ref and children): https://github.com/yewstack/yew/blob/master/crates/macro/src/html_tree/html_component.rs#L392

Tag:

html! {
   <li key={1} />
}

Macro code for parsing tag attributes: https://github.com/yewstack/yew/blob/master/crates/macro/src/html_tree/html_tag/tag_attributes.rs
We have a lot more special handling here for various keywords

Another thing to consider is that we may wish to enforce that keys are used whenever rendering inside lists. I think we can punt that for v2.

[drdozer]

@jstarry do you mean that we need keys within VList? So for example, make it pub children: Vec<OptionallyKeyed<VNode>>?

Presumably in general keys can be any K: PartialEq? So how do we deal with the polymorphism here? Is VList now parameterised over key type?

Do we need to allow the user to specify if keys are require, absent, or optional in a given context? This feels like a lot of cognitive overhead. But the alternative is that we always have Option<K> to deal with, and in some cases a component may want to require that children are keyed.

[jstarry]

@jstarry do you mean that we need keys within VList? So for example, make it pub children: Vec<OptionallyKeyed>?

Hmm, good question. I don't like the look of Vec<OptionallyKeyed<VNode>>. We probably want every VNode to implement a keyed() -> Option<String> method or something. We could add this to VDiff

Presumably in general keys can be any K: PartialEq? So how do we deal with the polymorphism here? Is VList now parameterised over key type?

I think we should use string keys to avoid that. So html! should accept keys that impl ToString

Do we need to allow the user to specify if keys are require, absent, or optional in a given context? This feels like a lot of cognitive overhead. But the alternative is that we always have Option to deal with, and in some cases a component may want to require that children are keyed.

Yeah this is more in line with what I am imagining. How about optional by default? I'm not sure how required keys would work at all yet. Might have to be a runtime check rather than compile time.

[jstarry]

@jeremyscatigna another thing to consider is that we will likely need keyed lists for this change too. Analogous to React keyed fragments

[ZainlessBrombie]

@jeremyscatigna whats your status? I would consider picking this up, it's a great opportunity to get into writing macros and I think this feature is important for yew :)

[deep-gaurav]

Hi I implemented a very rough implementation of key to components,
Available in my fork
https://github.com/deep-gaurav/yew

If no one is working on it I can try to polish and optimize it (by using hashmaps i suppose) and convert to a PR

key can be used on either tag or component
Eg

{ for self.state.terminals.iter().map(|tab| {
      html! {
          <div key=tab.1.title.clone() class={
              if tab.1.is_active{
                  ""
              }else{
                  "is-hidden"
              }
          }>
              <TerminalComp  termid=tab.1.title.clone()/>
          </div>
      }
    })
}

[jstarry]

@deep-gaurav I took a look at your changes and they look pretty great! I would love if you opened a PR for it :)

[deep-gaurav]

@jstarry Currently in my implementation for VDiff for every element in Vlist children a linear search is made for same key in ancestor's children, this can be very expensive for list with many children i think.
Can the Vlist children's type be changed from Vec<VNode> to HashMap<String,Vec<VNode>>

this way the unkeyed children will be in vec with key String::default(), and can be handled same way as before, and keyed children can be accessed directly with their key?

[jstarry]

Hm, it's important to keep the order of list children. Maybe a new data structure can be added with a map of key to children index?

[mrh0057]

@jstarry @deep-gaurav I made some changes to use a hashmap. I was wondering though if keyed should be Option instead of String on the nodes?
Link to the repo:
https://github.com/mrh0057/yew/tree/keyed_update

[mrh0057]

I was able to update the code to default to none on nodes without a key attribute.

[lukerandall]

@jstarry I believe this is closed by #1076

[jstarry]

Yes it is, thanks!