search

Matt-Esch / virtual-dom

A Virtual DOM and diffing algorithm
MIT License
11.65k stars 780 forks source link

Duplicate keys is not reported in a clear way #409

Open yminsky opened 7 years ago

yminsky commented 7 years ago

Instead, it just leads to incorrect patches being created, which causes things to blow up farther down the line at some unspecified point in time.

It would be nice if duplicating keys would lead to an exception that explicitly flagged this problem.

Even better would be for the diffing to be done in a correct, if less efficient way, in that case.

bendrucker commented 7 years ago

Duplicate keys as in duplicate keys in an object?

jgaskins commented 7 years ago

Issue is not reported in a clear way ;-)

yminsky commented 7 years ago

The problematic case is where you have two different vnodes with the same key. This causes the diffing algorithm to do the wrong thing (dropping nodes that shouldn't be dropped), silently.

jgaskins commented 7 years ago

There's still a bit of ambiguity here. Can you show an example?

yminsky commented 7 years ago

Sure. I have some examples I can put here in the morning. Just to be clear, though, the issue comes up specifically when you reuse the same key in multiple vnodes that have the same parent. By the key, I simply mean the argument called "key" in the constructor, here:

https://github.com/Matt-Esch/virtual-dom/blob/master/vnode/vnode.js#L12

yminsky commented 7 years ago

Here's a concrete example.

vdom1: https://gist.github.com/yminsky/e01531a14bc96e9b0bb474cc07a43aa6 vdom2: https://gist.github.com/yminsky/3933efc5faf4eef314ecdeea5c0e89bf patch: https://gist.github.com/yminsky/b9d6dc4e5e4ab67e8b9c2e6bfe728a14

The patch is the patch between vdom1 and vdom2, but setting the DOM to vdom1 and then applying the patch yields a different state than setting it to vdom2 directly.

I got these patches by dumping the JSON from the browser.