Keyed DOM-nodes get rerendered when they shouldn't? #66

cjh9 · 2017-12-08T19:38:51Z

Take a look at these runnable examples, one written with petit-dom and the other with picodom:

Picodom

<body>
<script src="https://unpkg.com/picodom"></script>
<script>
var { h, patch } = picodom

list = ["text0", "text1", "text2", "text3", "text4", "text5", "text6"];
oldnode = null;

function render() {
  node = h("div", {}, listElements());
  patch(oldnode, node, document.body);
  oldnode = node;
}

const listElements = () =>
  list.map((txt,idx) => {
    return h(
      "div",
      {
        onclick: () => {
          list.splice(idx, 1);
          render();
        },
        onupdate(e){
        	console.log(e)
        },
        key: txt
      },
      [txt]
    )});

render();
</script>
</body>

Petit-dom

<body>
<script src="https://cdn.jsdelivr.net/npm/petit-dom@0.0.14/dist/petit-dom.min.js"></script>
<script>
var { h, mount, patch } = petitDom

list = ["text0", "text1", "text2", "text3", "text4", "text5", "text6"];
oldnode = h('div', {}, ['first']);
document.body.appendChild(mount(oldnode));

function render() {
  node = h("div", {}, listElements());
  patch(node, oldnode);
  oldnode = node;
}

const listElements = () =>
  list.map((txt,idx) => {
    return h(
      "div",
      {
        onclick: (e) => {
          var i = idx
          list.splice(idx, 1);
          render();
        },
        key: txt
      },
      [txt]
    )});

render();
</script>
</body>

Open Chrome devtools, inspect elements and click on any text label do remove it, notice that the algorithm in picodom applies dom-changes to all the text-labels after the actual label (see animated gif below). They are also logged with the onupdate hook. But with petit-dom they are not. Is this expected behavior?

The text was updated successfully, but these errors were encountered:

jorgebucaran · 2017-12-09T02:11:19Z

@cjh9 Could you please write more readable code? I could edit your post and format the code for you, but then that's kind of disrespectful to the maintainer of the project.

cjh9 · 2017-12-09T17:09:06Z

@jorgebucaran Sry, just updated the code and ran it trough prettierJS :) Ok?

zaceno · 2017-12-11T10:34:08Z

@cjh9 Yes currently that's the behavior. If I understand what's going on correctly, it's not a bug, but a potential optimization to the patch-algorithm.

It's been discussed elsewhere (can't recall where now though), but the idea is that we should identify how much of the beginning and end of the list that hasn't changed, and leave those parts alone. Only apply the keyed patching algo to the middle-part where there have been changes.

(@jorgebucaran do you know the issue where the "prefix/suffix" thing was discussed? That's what this is about I think. Or?)

jorgebucaran · 2017-12-11T11:50:46Z

jorgebucaran/hyperapp#216

jorgebucaran · 2017-12-11T19:23:26Z

@cjh9 :) Not a bug! The vnodes are just being touched by setElementProp, we are setting onclick and onupdate handlers on them on every render.

cjh9 · 2017-12-12T06:49:21Z

@zaceno @jorgebucaran I've just debugged the thing and it looks like it is just reattaching the event listeners, so the dom-nodes are not recreated.

However this is not why the nodes are flashing, try attaching an event listener in devtools $0.onclick = function(){console.log('hello')}, nothing will happen. They are flashing because the algorithm are changing their order with insertBefore in patchElement. So this has a side effect, animations will be triggered on keyed element that should not be touched.

<body>
<script src="https://unpkg.com/picodom"></script>
<script>
var { h, patch } = picodom

list = ["text0", "text1", "text2", "text3", "text4", "text5", "text6"];
oldnode = null;

function render() {
  node = h("div", {}, listElements());
  patch(oldnode, node, document.body);
  oldnode = node;
}

const listElements = () =>
  list.map((txt,idx) => {
    return h(
      "div",
      {
        onclick: () => {
          list.splice(idx, 1);
          render();
        },
        class:'item',
        key: txt
      },
      [txt]
    )});

render();
</script>

<style>
@keyframes ANIMATION {
  0%   { opacity: 0; }
  100% { opacity: 1; }
}

.item {
  animation: ANIMATION 1s 1; 
}
</style>
</body>

zaceno · 2017-12-12T07:37:07Z

@cjh9 Ah I see. And that is something I think could only be fixed with an algorithm that ignores the unchanged beginning and end of a list. The "prefix/suffix" discussion from jorgebucaran/hyperapp#216

jorgebucaran · 2017-12-12T07:49:12Z

@cjh9 Thanks for figuring it out why stuff was flashing! 👍

jorgebucaran · 2018-02-27T16:14:31Z

jorgebucaran/hyperapp#499

jorgebucaran · 2018-06-29T08:18:57Z

This problem is now fixed after backporting jorgebucaran/hyperapp#663 from Hyperapp.

Thanks, @SkaterDad! 🎉

jorgebucaran added the bug Something isn't working label Dec 12, 2017

jorgebucaran closed this as completed Feb 27, 2018

jorgebucaran added PERFORMANCE and removed bug Something isn't working labels Feb 27, 2018

cjh9 mentioned this issue Apr 17, 2018

Work with any virtual-dom library zaceno/hyperapp-transitions#11

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyed DOM-nodes get rerendered when they shouldn't? #66

Keyed DOM-nodes get rerendered when they shouldn't? #66

cjh9 commented Dec 8, 2017 •

edited

jorgebucaran commented Dec 9, 2017

cjh9 commented Dec 9, 2017

zaceno commented Dec 11, 2017

jorgebucaran commented Dec 11, 2017

jorgebucaran commented Dec 11, 2017

cjh9 commented Dec 12, 2017

zaceno commented Dec 12, 2017

jorgebucaran commented Dec 12, 2017

jorgebucaran commented Feb 27, 2018

jorgebucaran commented Jun 29, 2018 •

edited

Keyed DOM-nodes get rerendered when they shouldn't? #66

Keyed DOM-nodes get rerendered when they shouldn't? #66

Comments

cjh9 commented Dec 8, 2017 • edited

Picodom

Petit-dom

jorgebucaran commented Dec 9, 2017

cjh9 commented Dec 9, 2017

zaceno commented Dec 11, 2017

jorgebucaran commented Dec 11, 2017

jorgebucaran commented Dec 11, 2017

cjh9 commented Dec 12, 2017

zaceno commented Dec 12, 2017

jorgebucaran commented Dec 12, 2017

jorgebucaran commented Feb 27, 2018

jorgebucaran commented Jun 29, 2018 • edited

cjh9 commented Dec 8, 2017 •

edited

jorgebucaran commented Jun 29, 2018 •

edited