-
Notifications
You must be signed in to change notification settings - Fork 24
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
RFC: List rendering reform #84
base: master
Are you sure you want to change the base?
Conversation
|
||
The above will throw a compile-time error, since otherwise the behavior may be unexpected and confusing, especially when considering the `lwc:key` on the same element. (Which `foo` does `lwc:key={foo.id}` refer to above? Rather than guessing, we will throw a compile-time error.) | ||
|
||
### `lwc:key` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One usecase that is not very clear about existing for:each
is nesting iteration blocks, where the inner for:each
directive is an immediate child of the outer one. What should be the behavior of the lwc:key
in that case?
IMO, this semantic makes it more clear than the older one. One thread related this confusion
1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. With lwc:key
on the same element as lwc:each
, you know exactly where the key should be applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need to slightly distinguish the way lwc:key
is applied vs key
. With for:each
when the short-hand is used, the key
is applied to the element containing the for:each
:
<li for:each={items} for:item="item" key={item.id}>
{item.name}
</li>
In the example, the key is applied to the li
and not the {item.name}
, whereas if it were a template
the key
would be applied to the children inside the template
.
The reason I mention this is because when there is a nested loop where the outer loop is a template
the key
is only applied once:
stackblitz example (playground link)
<template>
<template for:each={contacts} for:item="contact">
<div for:each={contact.addresses} for:item="address" key={contact.id}>
<section>{address.zip}</section>
</div>
</template>
Note the key
only appears on the div
, applying a key
on the section
would produce a warning.
If I understood correctly, with lwc:each
the new format would be:
<template>
<template lwc:each={contacts} lwc:item="contact" lwc:key={contact.id}>
<div lwc:each={contact.addresses} lwc:item="address" lwc:key={address.id}>
<section>{address.zip}</section>
</div>
</template>
How would the lwc:key={contact.id}
and lwc:key={address.id}
be applied in this scenario?
As long as there's a key on the div
I don't think it's a big issue but might be slightly unclear which one is applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmsjtu I think the example you provide is a bug in for:each
. It doesn't make any sense to me that one loop must have a key
, but the other loop must not have a key
.
In the case you mention, I think the problem arises from the ambiguity of what, exactly, the key
applies to:
<template for:each={contacts} for:item="contact">
<div key={contact.id} for:each={contact.addresses} for:item="address"></div>
</template>
In the above example, does the key
correspond to the outer (non-shorthand) template
loop? Or to the inner (shorthand) div
loop? It's ambiguous.
Another benefit of this RFC, IMO, is that lwc:key
is never ambiguous. It always applies to exactly the loop on the same element, regardless of shorthand or no-shorthand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it LGTM
|
||
The new `lwc:each` directive is essentially a superset of `for:each`, so it can leverage most of the teaching material for that directive. | ||
|
||
In addition, we can downplay or remove documentaiton for `for:each` and `iterator:*`, simplifying the LWC education narrative. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also mark for:each
and iterator:*
as deprecated to discourage usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be a bit early to do this. Similar to if:true
and if:false
, we need to make sure first that we have a well-lit migration path. But we can figure this out later.
Co-authored-by: Eugene Kashida <ekashida@gmail.com>
|
||
Similar to `key`, `lwc:key` is required on elements with `lwc:each`, and throws an error if missing: | ||
|
||
Missing lwc:key for lwc:each. Iterators must have a unique, computed key value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to discuss whether or not we keep this constraint with the new syntax. IMO, we should make it optional but strongly encouraged. The main reason we ask developers to set a key on iteration is to improve list diffing and re-ordering.
However, components don't always render a list of keyed items. It is currently awkward for developers to render a list of text in LWC.
export default class extends LightningElement {
tags = ["performance", "bug fix", "trust"]
}
To wrap around this limitation. developers usually use the text itself as a key.
<template for:each={tags} for:item="tag">
<div class="tag-wrapper" key={tag}>{tag}</div>
</template>
However, this approach is problematic as it only works if the list contains unique values. To solve this issue, I have seen some LWC components generating random ids for each entry which is also not ideal.
Now that LWC developers are used to key their iterations, I don't think that it would be problematic to relax this restriction. As a side note, I am not aware of any other UI framework enforcing this restriction.
Update: I see that it is already discussed in "Making lwc:key
optional".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also discussed in #84 (comment).
|
||
#### Variable reuse | ||
|
||
All new directives that take a string as their value (`lwc:item`, `lwc:index`, `lwc:first`, `lwc:last`, `lwc:even`, and `lwc:odd`) must have unique values when used on the same element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts on using {}
to wrap the lwc:item
instead of ""
? It's something that I have always found inconsistent in the current syntax.
In LWC template syntax, the usage of quotes on an HTML attribute denotes a static binding. But in the case of lwc:item
, it is used to indicate a dynamic binding to a variable.
For context, I was the one who originally pushed to wrap the lwc:item
value within quotes to discourage users from using JavaScript expressions. That said, my view changed on this over the years. I would like to get the sentiment from the rest of the team on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using ""
would make the syntax more consistent with scoped slots:
<c-child>
<template lwc:slot-data="item">
<span>{item.name}</span>
</template>
</c-child>
In my mind, the quotes mean "declare a variable with a given name," not "evaluate a JS expression." Also, if we change lwc:item
, we would arguably want to change lwc:index
, lwc:first
, lwc:last
, lwc:even
, and lwc:odd
as well, since they also declare a variable. For those reasons, I'm in favor of keeping ""
.
|
||
The implementation also becomes more complex, as we will need to support all three directives for the foreseeable future. Perhaps they can share logic under the hood, but we will still need to test all three, at the very least. | ||
|
||
We will also need to make an effort to advocate for adoption of the new directive, which may involve updating existing documentation and writing codemods or IDE tooling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts about adding compiler warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think similar to lwc:if
/lwc:else
, it may be a bit early to add compiler warnings. Compiler warnings to me should (normally) be reserved for truly broken things (like using a key
outside of an iteration, or invalid HTML syntax that is allowed by old parse5 versions). In the future, though – why not?
- We have two directives for lists instead of one, with different features available on each. The new directive unifies them. | ||
- The existing directives do not use the `lwc:` prefix, unlike every other modern LWC directive. Using `lwc:` makes it clear which attributes are LWC-specific, and would allow for improvements in the future such as [shorthands](https://github.com/salesforce/lwc/issues/3303) (out of scope for this RFC). | ||
- The key should be defined on the list root element, not on each element inside the list. The current behavior requires tediously repeating the `key` for each element inside the list, and also leads to [inconsistent behavior](https://github.com/salesforce/lwc/issues/3860) for text/comment nodes. | ||
- There is no way to easily render based on even vs odd list items. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmdartus raised another benefit: we could use Fragments as the root of each list item. This should make the vdom diffing much more efficient because we would be diffing fewer vnodes in cases where the list repeater has multiple top-level elements.
Today we can't do this due to key
. A developer could technically put a different key
on every top-level element inside of a list, which would defeat the fragment optimization since the fragment must have one key. The new proposal solves this since lwc:key
is hoisted.
|
||
- [Vue `v-for`](https://vuejs.org/guide/essentials/list.html) | ||
- [Svelte `{#each}`](https://svelte.dev/docs/logic-blocks#each) | ||
- [Angular `*ngFor`](https://angular.io/api/common/NgForOf#local-variables) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Angular has count
which is not in this proposal. Maybe it's worthwhile for iterables/iterators where you don't easily have array.length
.
2. It deviates from the existing behavior of `for:each`, and is one more thing to teach. | ||
3. Developers may now assume that keys are actually a _bad_ idea, since the "improved" `lwc:each` makes them optional. | ||
|
||
So, for this proposal, keys are still required. However, it is definitely something that can be revisited in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmdartus: Use case for making it optional:
- You have a list of tags, it's just a list of strings, they'll never change
- You use the tag itself as the key since the key is required
- Now imagine suddenly tags are duplicated, now you need a unique id for each tag
- Now you have to add special IDs or
Math.random()
to work around it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekashida: We could add another directive like lwc:no-key
to communicate that a key is not required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or lwc:dangerously-set-no-key-this-will-be-slow-and-i-like-it-that-way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into how other frameworks do this:
- Svelte/Vue/Angular: keys are optional (Angular calls it
TrackByFunction
rather than "key") - React: keys are not required, but you will get a console warning without them
- Lit: keys are effectively optional: you can use
Array.prototype.map
for the key-less approach, or therepeat
directive which requires akeyFunction
- Solid: More subtle.
<For>
does not require an explicit key function by design since Solid already has fine-grained reactivity. However it is keyed based on referential equality. Whereas there is the alternative<Index>
component which is non-keyed and designed for cases where referential equality doesn't make sense (such as primitives).
|
||
### Prior art | ||
|
||
- [Vue `v-for`](https://vuejs.org/guide/essentials/list.html) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing from this proposal: Vue has a way of iterating over objects (key/value) as well as array-likes. Given that Object.entries()
exists, though, I'm not sure how valuable this is.
I have a positive sentiment toward this reform. |
Rendered