Skip to content
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

feat: make repeat directive optionally accept a key #6919

Open
sknoslo opened this issue Mar 1, 2024 · 4 comments
Open

feat: make repeat directive optionally accept a key #6919

sknoslo opened this issue Mar 1, 2024 · 4 comments
Labels
area:fast-element Pertains to fast-element community:request Issues specifically reported by a member of the community. improvement A non-feature-adding improvement status:needs-investigation Needs additional investigation

Comments

@sknoslo
Copy link
Contributor

sknoslo commented Mar 1, 2024

🙋 Feature Request

Add an optional key to the repeat directive, so that repeat can map a DOM node to a specific item when updating views. Similar to how React or Lit work. This would be beneficial for a couple of reasons:

  1. DOM nodes used within a repeat can maintain their own internal state.
  2. Assistive technology, such as a screen reader, will be able to better understand when content has changed.
  3. Potentially less expensive for some operations?

🤔 Expected Behavior

When an array is sorted, or items are added or removed, existing DOM nodes should only be reused for an item with the same key. New DOM nodes should be created for keys that did not previously exist, and DOM nodes should be removed for keys that no longer exist. DOM nodes should never be reused for items with different keys.

😯 Current Behavior

When an array is sorted, or items are added or removed, existing DOM nodes are reused just based on position, and the nodes do not map 1-to-1 with the array elements.

💁 Possible Solution

Add a optional key function to the repeat directive. Maintain a map of keys -> DOM nodes, and make updates accordingly. Might look something like:

repeat(
  x => x.items,
  html`<div>${item.uniqueId}</div>`,
  { keyFn: (item) => item.uniqueId }
)

Adding a separate "keyed repeat" directive could also be a good option if it makes more sense to keep the behavior distinct.

🔦 Context

I was building a notification component that could stack alerts to display for a short time. These elements had role="alert" so that a screen reader would announce them when they showed up. But, because nodes are recycled, as new alerts show up or were dismissed, the contents of existing nodes change and the screen reader announces them again, as if they are actually new.

I had to fall back to manually managing the DOM instead of using a template with the repeat directive.

💻 Examples

Comparison of behavior between Lit and FAST on StackBlitz. This is a contrived example where a parent element renders a list of numbers in a child element, and the color of the child element can be toggled. This toggle state is owned by each child element. If you toggle the first (or last) item in the list, and then reverse the list, you'll see how the behavior differs.

@sknoslo sknoslo added the status:triage New Issue - needs triage label Mar 1, 2024
@chrisdholt chrisdholt added status:needs-investigation Needs additional investigation area:fast-element Pertains to fast-element and removed status:triage New Issue - needs triage labels May 2, 2024
@imink
Copy link

imink commented Jun 4, 2024

More info, If you change the repeatable component to be pure component (do not own its own state), it works.
https://stackblitz.com/edit/vitejs-vite-prtfgs?file=src%2Ffast-repeater.ts

@imink
Copy link

imink commented Aug 20, 2024

To simplify, we need repeat directive maintainning state with Key, just like vuejs or lit.
https://vuejs.org/guide/essentials/list.html#maintaining-state-with-key
https://github.com/lit/lit/blob/ff87eb461dfbfa8fd0101a6a9067dcaaa9b49f92/packages/lit-html/src/directives/repeat.ts#L114

This is vital for cases loopping complex components that has its own state, i.e. treeview component with virtualization.

@chrisdholt chrisdholt added improvement A non-feature-adding improvement status:planned Work is planned community:request Issues specifically reported by a member of the community. status:needs-investigation Needs additional investigation and removed status:needs-investigation Needs additional investigation status:planned Work is planned labels Oct 18, 2024
@chrisdholt
Copy link
Member

Thanks for this - We hope to be able to pick this up barring anything that comes up during our investigation!

@HcySunYang
Copy link

Hi all, how is this matter going so for? The recycle is very buggy, we have to disable it right now. But if we disable it, more issues are going to be introduced, for example, the focus management, since the DOM is destroyed and recreated, the focus will be lost...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:fast-element Pertains to fast-element community:request Issues specifically reported by a member of the community. improvement A non-feature-adding improvement status:needs-investigation Needs additional investigation
Projects
None yet
Development

No branches or pull requests

4 participants