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.

Sign up for GitHub

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

Jump to bottom

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

Closed
cjh9 opened this issue Dec 8, 2017 · 10 comments
Closed

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

cjh9 opened this issue Dec 8, 2017 · 10 comments

Comments

@cjh9
Copy link

cjh9 commented Dec 8, 2017 •
edited
Loading

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/[email protected]/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?

out
@jorgebucaran
Copy link
Owner

@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
Copy link
Author

cjh9 commented Dec 9, 2017

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

@zaceno
Copy link
Contributor

zaceno commented Dec 11, 2017

@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
Copy link
Owner

jorgebucaran/hyperapp#216

@jorgebucaran
Copy link
Owner

@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
Copy link
Author

cjh9 commented Dec 12, 2017

@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>

out

@jorgebucaran jorgebucaran added the bug Something isn't working label Dec 12, 2017
@zaceno
Copy link
Contributor

zaceno commented Dec 12, 2017

@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
Copy link
Owner

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

@jorgebucaran
Copy link
Owner

jorgebucaran/hyperapp#499

@jorgebucaran jorgebucaran added PERFORMANCE and removed bug Something isn't working labels Feb 27, 2018
@cjh9 cjh9 mentioned this issue Apr 17, 2018
Closed
@jorgebucaran
Copy link
Owner

jorgebucaran commented Jun 29, 2018 •
edited
Loading

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

Thanks, @SkaterDad! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jorgebucaran @zaceno @cjh9