Added computed keys based on attribute names to prevent reinsertion on prepend

[Wildhoney]

I don't want this merged — only for discussion. All of the unit tests are passing, however I'm sure this change has unintended consequences that are not covered by the tests.

Changes I'm proposing are to add a computed key based on the passed attributes — keys only as values are more likely to change between renders. If attributes are added/removed between renders then the element will be reinserted and can be solved by specifying a key manually, but I do believe that attributes are generally consistent between renders – only their values will differ.

Due to above change I've had to modify one of the tests where an attribute was removed (xlink:href). It was failing because an attribute was removed and so its key differed. Solutions to this are:

• Specify the key manually (chosen solution for the test);
• Select the use again (via querySelector) as it's a new element;
• Be explicit and add "xlink:href": null to the attribute list;

[codecov-io]

Codecov Report

Merging #152 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #152   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         183    184    +1     
  Branches       52     52           
=====================================
+ Hits          183    184    +1

Impacted Files	Coverage Δ
src/index.js	100% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4691379...50e8d83. Read the comment docs.

[leeoniya]

some notes:

• Object.keys() is going to noticeably impact performance (double whammy when calling it twice).
• key ? key : won't handle the case of 0 properly, you'll probably want key != null ? key :

[Wildhoney]

It’s a proof of concept to open the discussion, which is why I said I didn’t want it merged. I’m looking more for feedback on the consequences of the proposed change. It of course adds additional overhead which could be an opt-in, but that goes against the creator’s philosophy which I respect.

[zaceno]

We could talk about the unintended consequences of this, but first: what are the intended consequences? What is the benefit of this?

[jorgebucaran]

@zaceno https://github.com/jorgebucaran/superfine/issues/151 @Wildhoney Maybe we can solve this without pre-computed keys. Perhaps by peeking at the next element in the list, but that's still limiting.

[Wildhoney]

But what do you mean by limiting?

I think there's only so far we can peek into the tree to reuse a node. Ideally the search should be restricted to the current subtree.

Although the tests pass, I think my concern with this approach is that using either the pre-computed key approach, or the peeking around the subtree to find an element that matches either by name, or by name and attributes/partial-attributes, we lose the reliance on the position of an element. Therefore there are plausible instances where multiple elements may be found to be a contender for reuse. If so, what happens and what are the consequences?

For example something like:

onSecondPass && h('div');

h('div', { a: '-', b: '-' }, [
    'One'
]);

// Duplicate key, so does it attempt to patch the same element as above? Thus
// rendering "Two" in the previous element.
h('div', { a: '-', b: '-' }, [
    'Two'
]);

[frenzzy]

The goal is to make diff algorithm smarter and do not force user to specify keys when obviously different nodes are used:

<main>
  {checked && <h1>✔</h1>}
  <input type="checkbox" />
</main>

vs

<main>
  {checked && <h1 key="h1">✔</h1>}
  <input key="input" type="checkbox" />
</main>

I think the right solution here is to update diff algorithm to check both VNode key and name. Keys should matter only when sibling nodes have the same names, IMO.

[zaceno]

I thought the vdom algorithm already differentiated by tagName?

[jorgebucaran]

Assuming checked was true, the old children will consist of two nodes and the new children of one node. Keying the text input only makes the algorithm reuse the existing node instead of replacing it with a new one.

@zaceno Correct, here.

https://github.com/jorgebucaran/superfine/blob/4691379acb9ff250c1e42e66f959a25942e286ad/src/index.js#L189-L197

Maybe @frenzzy means in the keyed reconciliation algo? or something else.

[zaceno]

The way I understand this PR, is to avoid having to use keys, with the assumption that nodes with the same attributes represent the same element. But that's not true in many cases.

<ul>
  <li>...</li>
  <li class="highlight">...</li>
  <li>...</li>
  <li>...</li>
</ul>

In this case the idea that the node with class="highlight" is the same element is clearly incorrect, although there isn't much harm in behaving as if

... unless you are also using lifecycle events to do things with the actual dom elements. Then it gets very important not to mistake which element is which.

[frenzzy]

This example currently does not work as expected (try to fill and clear input, input loses focus): https://codepen.io/frenzzy/pen/dgmzWE/left/?editors=0010

You have to use keys to make this example work. Diff algorithm could be smart enough to not re-create the input because different tags are used.

[jorgebucaran]

@frenzzy Anyhow, the problem is that the label is only present in the new children. We're patching null|node and then node|(node,node). Because no keys were used, the last patch compares the label with the text input and seeing that they're not of the same type (name) the input is replaced with a new label. Then a new input is created and appended to the list.

[frenzzy]

Exactly! And if diff algorithm could look not only on keys but also on tag name, it could patch correctly without keys!

[jorgebucaran]

@frenzzy The problem is the number of nodes in the old and new children arrays. The non-keyed algorithm works by comparing nodes positionally. When patch attempts to replace input with label input this happens:

- input
+ label
+ input

What you want is instead:

+ label
input

But the algorithm doesn't know you want to insert a label in front of the input, rather, it expects the input to be a label, and because their types (tag name) don't match, it replaces the old input with a label and proceeds to the next node (the input) in the new children array. At this point, we've covered all the nodes in the old children array, so we just create a new input and append it to the parent.

So, the goal is to insert elements efficiently even if you are not using keys. How do we do that?

[jorgebucaran]

Before we replace an element, we can peek at the next node we want to create and compare it with the current old node. Or reproduce part of the keyed algorithm but check only for names.

Sounds like a lot of work when keys were precisely introduced to manage cases like these.

[frenzzy]

No keys

- input
+ label
+ input

With keys

- label key="a"
  input key="b"

This PR (keys set automatically if user missed them)

- label key="label"
  input key="input"

[jorgebucaran]

Actually, name + Object.keys(props) as key according to this PR.

What about just name? Still, why not just use keys?

[frenzzy]

I think this PR only shows an idea, but the solution is not good enough. For example in case of multiple similar nodes algorithm will probably render only one node instead of 4:

- li key="li"
- li key="li"
- li key="li"
  li key="li"

[jorgebucaran]

Exactly. Maybe there's another solution. Does anybody know what Preact, React, Vue, and so on do?

[frenzzy]

React.js diff algorithm takes into account differences in node types (label vs input), not only in keys. Demo: https://codepen.io/frenzzy/pen/dgmJmx/left/?editors=0010

[zaceno]

Ah I get it now. Although the diffing also does take into account vnodes' tag-names, it makes no attempt to reuse/reorder them, unless they are keyed.

I'm not sure that's a huge problem, because anytime it matters wether an element is recreated or not, you should use a key. You should use a key because, I don't think there's an algo that can dependably know wether you intend to keep an element or replace it, in all cases.

Anyhow. Even though this is not a significant problem in my book, if we are going to solve it, I don't believe the solution here is the correct one. Especially if it does not prevent non-unique keys being calculated.

[mindplay-dk]

I think I've suggested this in the past: why not simply auto-populate key based on the name property? if no key is defined, using a calculated key should help with diffing.

So calculate it as e.g.: key = key || (name + "#" + (index[name] = (index[name] || 0) + 1))

That is, use a calculated key identical to the name plus a unique index for that name in that parent.

The index ensures that e.g. input type="checkbox", where the same name occurs multiple times, gets unique sequential keys, which should work for most use-cases.

Note that this auto-key should not be applied to a tags, where name means something different.

(or maybe an auto-key should only be applied to e.g. input|textarea|select)

[jorgebucaran]

Superfine 7 is the simplest Superfine yet and I don't want this in the box. Thanks for all the words! :)