Hyperapp: using shared nodes in the function view can induce wrong display.

[gbagan]

An example here:

a minimal example here:
https://jsfiddle.net/ecmLpgw8/
The counter 2 is incremented while it must be the counter 1.

The problem is the algorithm "patch" which affects a dom node to a virtual node my mutate its attribute .node. The algorithm does not manage cases where two virtual nodes have referential equality.

I think a way to fix this bug is to affect the virtual node to the dom node ( node.__vdom = vdom ) and adapts the algorithm accordingly.
The reason it that It can have several dom nodes associated with a virtual node but only one node associated with a virtual node.
Maybe it can produce performance issues, I don't know.

I agree that the example is an uncommon usage but It think it is remains a bug.
Notably, it breaks the concept of referential transparency.
If the fix involves too much effort or prdouce too much performance issues, I am ok to not fix it.

[gbagan]

Another example. I try to make it more realistic.
https://jsfiddle.net/tu9s716x/1/

[jorgebucaran]

@Guillaume this example is trivial, but we can imagine openedDiv and closedDiv need a lot of time to be computed

Sounds like a job for lazy views! https://github.com/jorgebucaran/hyperapp/issues/721

And you're also trying to optimize the wrong process. Computing nodes is never the bottleneck.

import { h, app } from "hyperapp"

app({
  init: { items: [false, false, false, false, false] },
  view: state =>
    h("main", {}, [
      state.items.map(isOpen =>
        isOpen ? h("div", {}, "open") : h("div", {}, "close")
      ),
      h(
        "button",
        {
          onClick: ({ items }) => ({
            items: items.map((x, i) => (i == 3 ? !x : x))
          })
        },
        "open div 3"
      )
    ]),
  node: document.getElementById("app")
})

[gbagan]

Actually, it is more complex than I was thinking.
I think it can be fixed as follows:
we use a variable id that determines the render number. It starts from 0 and it is incremented at each render.
the vnodes contain an attribute .$id that will be mutated during the render.
getVNode need to be modified:

    if vnode.$id < current render id
        (that means that the vnode has not seen before during the current render)
    then
        change nothing and vnode.$id = current render id
    else make a shallow copy of the vnode 

That prevents problem with shared vnodes but keep optimization for lazy views

[gbagan]

I have thought and thought about how to fix this bug.
I think the only way to prevent strange behaviour when parts of the vdom are shared is to make not mutate the vdom returned by the function view.
I have started to write a patch.
The idea is to not store nodes into vnodes but in a structure that contains both vnodes and nodes.
This structure is mutable but vdom is immutable.
I have called it ptc but it just a temporary name it is a array of 3 elements [node, array of ptc children, vdom]
My tentative is here. It is not finished.
Just to give an idea.
https://github.com/gbagan/hyperapp/commit/467411e3c4e44341257c6998e72910285650dd10

[sergey-shpak]

@gbagan @jorgebucaran ,
hyperapp shouldn't mutate vdom, and the fix looks quite easy to me:

var getVNode = function(newVNode, oldVNode) {
  // everything stays the same, except return new node
  // was : newVNode, and now:
  : Object.assign({}, newVNode)
}

creating and working with shallow copy of each vdom node is fixing 'shared nodes' or any other 'vdom mutation' issue, also this looks more 'functional' way of 'not mutating any external'.
Any thoughts?

[gbagan]

Just seen and it was my first idea.
But that breaks all optimizations based on comparing two vnodes. Lazy view, for example.

[sergey-shpak]

@gbagan there is no strict comparison between vNode objects, they compared by properties (name, attributes, etc), so creating and storing shallow copy of vNode doesn't impact on comparing vNodes.

Shallow copy of vNode still contains all the vNode props (lazy view, props, etc) and can be compared with new vNode the same way as before (without loosing any optimizations), but at the same time it allows to not mutate external vNode objects (solves 'shared nodes' or any other vnode mutation issue)

[gbagan]

In the function patch, the algorithm tests the two vnodes and skips all the function if the test succeeds.
That permits to skip whole subtrees

if (oldVNode === newVNode) {
  }

[sergey-shpak]

@gbagan generally talking, this construction if (oldVNode === newVNode) {} is confusing and doesn't work as you expect, it can be omitted because:

1. Hyperapp expects that view function always returns new vdom tree (otherwise patching doesn't have any logical sense)
2. This construction compares only object references (not theirs props), so even though view returns the same vdom reference it has to be passed into patching algo, because attributes or children could be changed and those changes has to be reflected into the dom

I can be wrong, but this construction comes more from 'writing code style' and doesn't solve any real case @jorgebucaran please confirm

[gbagan]

Maybe you are right.
For me, the main use is in combination with lazy view.
If the lazy view returns the same view than before, then patch can skip all the subtree returned by the lazy view.
The function patch always return something the return is outside a if/else statement

[gbagan]

Unless I am wrong, if two vnodes have the same reference, there children and attributes are necessarily the same. But the inverse is not true.
edit:
Actually under the assumption we are in a FP-style.
If we mutate directly the subtree, it is possible.
But if we does not mutate a tree, any modification on the leaf implies a modifictation of the root.

[sergey-shpak]

ok, let's clarify this

For me, the main use is in combination with lazy view.
If the lazy view returns the same view than before, then patch can skip all the subtree returned by the lazy view.

It doesn't work this way, you shouldn't store and return the same lazy view vdom to skip lazy view rendering. Lazy view rendering depends only on lazy props, if they have changed - hyperapp rerenders lazy view and patches the dom, if not - rendering/patching is omitted.

But while you were storing and returning the same vdom tree to omit patching dom (imo, which is wrong lazy usage) you have found the issue with 'shared vNodes' mutations, which is easily solved with shallow vNode copy (as I already shown)

[gbagan]

It doesn't work this way, you shouldn't store and return the same lazy view vdom to skip lazy view rendering. Lazy view rendering depends only on lazy props, if they have changed - hyperapp rerenders lazy view and patches the dom, if not - rendering/patching is omitted.

That my point but I misspoked. Sorry. (I have not said that I store the vdom)
I would say
"If the lazy view returns the previous view "

Globally I had the same idea than you but I am convinced myself than it is not the good way to do (maybe I am wrong).
The question is: which vdom you keep after the render to use as previous vdom for the next render.
If you use the original vdom, you lose every information about dom nodes.
If you use the copy of the vdom, you lose the patch skipping (because you lose reference equality)

edit: maybe I have missed something about lazy views. I have thought that they work the same way as in Elm. I will check that.

reedit: as I understand in the function getVNode, lazy views always return something. Either the previous vdom if the argument have not changed or a new built view. So, unless I am wrong, reference equality in patch is still necessary.t

[sergey-shpak]

@gbagan ok, np, let's summarize the issue

1. Hyperapp mutates vNodes by storing reference to dom nodes
2. This mutation creates unexpected patching results when vNode is reused in vDom tree.

// simplified example
view: state => {
  const node = h('div', {}, [])
  // each of the nodes will share the same dom node
  return [ node, node, node ]
}

1. The simplest fix is to create shallow copy of each vNode internally
2. In general, hyperapp patching algo is using vNode props to compare nodes, and it should work the same way with shallow copy of vNode. But there is one patching case to mention if(oldVNode===newVNode){} which may not work with shallow copy of node as expected, but this case has to be omitted by following reasons

Edit: after discussion with @gbagan the if(oldVNode===newVNode){} case has to be double-checked, because it can affect Lazy

@jorgebucaran @zaceno @mrozbarry

[jimfranke]

@jorgebucaran @zaceno @mrozbarry What are the current thoughts about this issue? Is shallow copying and omiting the reference equality check a plausible solution? If this could work the solution would be a lot simpler than the proposed PR #904

[jorgebucaran]

@jimfranke I need to study this more carefully, but it seems like a user mistake, not a bug.

[zaceno]

The way I see it, we don't need to fix anything in hyperapp to allow reusing nodes, because it doesn't enable anything besides possibly optimization via memoization. But since that is essentially what Lazy is for, we're covered.

---

EDIT: I understand that referential transparency would be a good thing in principle, but it doesn't add any apparent practical benefits from what I can tell, and the proposed solution would likely decrease performance (running the benchmarks to verify this would be good!). For that reason I am for keeping things as they are.

EDIT2: @jimfranke Omitting the strict equality check to skip diffing of vtree-branches would break Lazy optimization. That's not a good trade-off imo.

[gbagan]

I agree with Zaceno comments.
For me, this is clearly a (minor) bug unless it ispecified somewhere that one cannot share vdom subtrees.
But, I agree this bug is minor and fixing it will probably decrease performance.
Since performance seems to be an important aspect of hyperapp, I am ok to not fix it.
And honestly, I am very busy these weeks and at the moment, I have no time to dedicate to this bug.