Skip to content

Support 'key' to guarantee preservation of elements/components #8232

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

Closed
SteveSandersonMS opened this issue Mar 6, 2019 · 2 comments
Closed
Assignees
Labels
area-blazor Includes: Blazor, Razor Components Components Big Rock This issue tracks a big effort which can span multiple issues Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Mar 6, 2019

This is inspired by, and would basically be equivalent to, the key feature in React. It provides a way to tell the system how you want it to preserve child element/component instances when diffing a list.

Use case

Consider the existing situation where you don't have this feature:

<div>
    @foreach (var flight in Flights)
    {
        <DetailsCard Flight="@flight" />
    }
</div>

If you add a new flight into the middle of the Flights list, what you'd want is that all the existing DetailsCard instances are unaffected, and one new DetailsCard gets created and put into the rendered output.

To visualize this, if Flights previously contained [F0, F1, F2], then this is the before state:

  • DetailsCard0, with Flight=F0
  • DetailsCard1, with Flight=F1
  • DetailsCard2, with Flight=F2

... and this is the desired after state, given we insert a new item FNew at index 1:

  • DetailsCard0, with Flight=F0
  • DetailsCardNew, with Flight=FNew
  • DetailsCard1, with Flight=F1
  • DetailsCard2, with Flight=F2

However, the actual after state with the present diff algorithm is this:

  • DetailsCard0, with Flight=F0
  • DetailsCard1, with Flight=FNew
  • DetailsCard2, with Flight=F1
  • DetailsCardNew, with Flight=F2

The system has no way to know that DetailsCard2 or DetailsCard3 should preserve their associations with their older Flight instances, so it just re-associates them with whatever Flight matches their position in the list. As a result, DetailsCard1 and DetailsCard2 rebuild themselves completely using new data, which is wasteful and sometimes even leads to user-visible problems (e.g., input focus is unexpectedly lost).

Fixing this with 'key'

The developer should be able to make their app more efficient (and sometimes, better behaving) using key like this:

<div>
    @foreach (var flight in Flights)
    {
        <DetailsCard key="@flight" Flight="@flight" />
    }
</div>

Here, key is a new built-in intrinsic that you can use on any component, or any element. The result of this will be that you do get the desired after state shown above.

To achieve this, the diff algorithm:

  • Notices if you've supplied a key, then from that point onwards within the current parent frame (in the above example, the <div>), it changes the rules for retaining child elements/components
  • In this mode, previous child elements/components are reused if and only if their old frames have the same key value as the new frames.
  • That is, use of key is a equally a way of guaranteeing the non-preservation of children that don't match existing frames
    • As a further improvement, we might want to refine this by saying we can preserve any instances where both old and new don't have any key specified, like the system already does. Not sure whether that will complicate the implementation. It would improve efficiency in cases where you have a lot of top-level elements/components within a @foreach block, and are only using key for some of them.
  • If the key value is an int, we can use and compare it directly (and can store such values on the RenderTreeFrame without additional boxing). For other types, we'll have to call GetHashCode to get an int. We can accept null and give it some arbitrary int value (e.g., 0). Alternatively, we could consider doing reference comparisons for other types, as the perf would be improved but it might surprise people that we don't respect GetHashCode.
  • If the key values aren't unique within a given parent, this is an anomalous case, and preservation behavior is undefined. We'll probably preserve the first instance for each nonunique key value, and then create new instances for the rest. As long as the output is legal (like it already is) we don't have to define the rules more precisely.
  • If the key values match a certain old and new frame, but those frames aren't candidates for retention (e.g., they are different types of child component or element), this is a anomalous case, and the system will do whatever's easiest to implement while still being legal diff output. For example, we might consider that key value to be "used up" even though we don't retain the child (because it was incompatible).

Implementation note: Naively, this can be achieved either by a nested loop (for each new output frame, scan the old frames to find one with a matching key, and somehow track which ones were used already), or by a hash join (while processing, build a dictionary of the old frames by key, then iterate over the output frames to attach the corresponding children or create new children when there isn't a match). However we would want to avoid allocations, so might need some shared dictionary within the diff context.

To be clear, even though the example above uses key on a component, you could equally use it on plain HTML elements, e.g.:

@foreach (var todo in TodoItems)
{
    <input key="@todo.Id" bind="@todo.Text" />
}

This is valuable because it ensures you preserve things like focus, cursor position, visibility state of tooltips, etc., if the system injects new items into the list while the user is interacting with it.

Why can't the system do this automatically? Why does the developer have to specify key explicitly?

Theoretically we could make this more automatic, using either of these techniques:

  • Within a @foreach (var x in ...) { ... } block, we could implicitly put a key=@x on the first top-level component/element in the block. Drawbacks:
    • Adding another top-level thing might break your app by changing retention behavior, which is surprising and weird
    • It adds diff cost in all cases, even when there's no benefit
  • Or, we make the default retention logic more sophisticated, so that even without any key, it tries to find the optimal old-new matches by diffing the attributes and picking matches to minimize edit distance.
    • I think it's obvious that we're not going to want to add this extra diff cost in all cases, especially given how it makes the behavior harder to predict. People would end up with really obscure, hard-to-repro bugs where the retention behavior switches around depending on user-entered data.
@SteveSandersonMS SteveSandersonMS added enhancement This issue represents an ask for new feature or an enhancement to an existing one area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-blazor Includes: Blazor, Razor Components labels Mar 6, 2019
@mkArtakMSFT mkArtakMSFT added this to the 3.0.0-preview5 milestone Mar 6, 2019
@MartinF
Copy link

MartinF commented Mar 13, 2019

I'm very happy to see that you have prioritized this and make an effort in including a solution.
It is quiet a headache to work around it. :)
It would be great if you could also somehow think about how to retrieve references to Components generated in a loop (I have described my initial thoughts in #7048).

@rynowak rynowak mentioned this issue Mar 25, 2019
56 tasks
@danroth27 danroth27 added the Components Big Rock This issue tracks a big effort which can span multiple issues label Apr 19, 2019
SteveSandersonMS added a commit that referenced this issue May 7, 2019
* Store component/element keys on RenderTreeFrame

Also refactored how RenderTreeFrame gets constructed. The previous arrangement of having ad-hoc ctor overloads for different scenarios became intractible (too many combinations to avoid clashes; risk of accidentally losing field values when cloning). There's now one constructor per RenderTreeFrameType, so you always know where to add any new field values, and implicitly guarantees you don't lose other field values because adding a new param forces updates at all the call sites.

* Add StackObjectPool, which will be useful momentarily

* Support keyed insertions/deletions

* Refactor AppendDiffEntriesForRange to prepare for adding "move" logic

* Apply permutations on the JS side

* Handle keyed moves by writing a post-edit permutation list

* Shrink KeyedItemInfo struct

* Include sourcemaps when building client-side Blazor apps with ReferenceFromSource

* Update struct length of edit frames now it's explicit layout

It's longer now because all the reference-type fields, except the last, now have to be 8 bytes for compatibility with 64-bit runtimes. Previously on Mono WebAssembly the reference-type fields were all 4 bytes.

* Tolerate clashing keys (i.e., produce a valid diff, even if suboptimal)

* Tolerate keys being added/removed incorrectly

* E2E test harness for 'key'

* Some more unit test cases

* Invert diffing logic to prefer matching by key over sequence

Previously it preferred sequence over key, but that's wrong, and surfaces as bugs when you mix keyed and unkeyed items. We need to prefer key over sequence, because key is meant to guarantee preservation, whereas sequence is just best-effort preservation.

* Make unit test cases more adversarial

* First actual E2E test

* In E2E test, verify correct preservation of components

* E2E tests for simple insert/delete cases (with and without keys)

* E2E test for reordering. Also extend other tests to verify simultaneous editing.

* E2E test for many simultaneous changes

* Update reference sources

* CR: Avoid x = y = z

* CR: Only use 'finally' for actual cleanup

* CR: Clean up RenderTreeFrame assignment

* CR: Include 'key' in RenderTreeFrame.ToString()

* CR: Avoid "new T()" in StackObjectPool

* CR: Make KeyedItemInfo readonly

* CR: Handle change of frame type with matching keys (and sequence)

* CR: Add E2E test showing form + key scenarios

* Preserve focus across edits

* Tweak E2E test case

* In client-side Blazor, prevent recursive event handler invocations

* Actual E2E tests for moving form elements
@mkArtakMSFT mkArtakMSFT added Done This issue has been fixed and removed 1 - Ready labels May 8, 2019
@mkArtakMSFT mkArtakMSFT removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 9, 2019
@SQL-MisterMagoo
Copy link
Contributor

This looks awesome!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components Components Big Rock This issue tracks a big effort which can span multiple issues Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

6 participants