Memoized elements are removed & added anyway

[timwis]

Hello, I'd like to cross-post the issue shama/on-load#10 as @shama and @yoshuawuyts have suggested the issue may reside in morphdom.

I reported that:

if you memoize your el, the onload callback gets called at every re-render.

@shama investigated, reproduced the issue, and diagnosed that:

For some reason each call to yo.update() is quickly removing and adding the same node. We could fix this here by queueing events, so if an load is called immediately after an unload, it ignores it. But morphdom shouldn't be updating the node in the DOM to begin with.

The following code sample illustrates the issue, with a live demo here:

var html = require('bel')
var morphdom = require('morphdom')

var saved
function child () {
  saved = saved || html`<h1 onload=${() => console.log('child on')} onunload=${() => console.log('child off')}>child</h1>`
  return saved
}

function parent () {
  return html`<div>${child()}</div>`
}

var root = parent()
document.body.appendChild(root)
setTimeout(() => {
  console.log('updating')
  root = morphdom(root, parent())
}, 1000)

Expected behaviour: child on only gets called once, when the element is mounted.

Actual behaviour: child on and child off are also called at re-render, even though the child element was already memoized.

This issue prevents things like memoizing leaflet/d3 components so they don't have to be re-initialized at every state change. Any suggestions on how we might resolve this?

[arturi]

Currently struggling with this too. This demo I made might help visualize the problem: http://requirebin.com/?gist=0f7bb3b03959448e120f02d506f4cd7e

[AutoSponge]

The element you're holding a reference to gets moved to the new tree when you pass the template to bel. You should be able to watch this happen if you put a breakpoint right before you call morphdom and after you call parent.

Because the reference is an element, it's unique and can't live in two places at once.

In your breakpoint, you should see the element disappear. When that happens, you get the unload event. When morphdom re-appends it, you'll get the load event.

[arturi]

Thanks. So can this be fixed/worked around? Or is storing an element in a variable a no-go for effective morphdom diff?

[AutoSponge]

If you don't try to memoize the element, just make child return an element, morphdom performs non-destructive updates and the handler only fires once (never fires for unload). If that doesn't work for you, I would need a more in-depth example to work with to understand what you're trying to do.

[arturi]

I was trying to solve this issue https://github.com/shama/bel/issues/26 by storing an image in a var as @yoshuawuyts suggested, so that a network request wouldn’t be made each time render is called:

I have an example here: http://requirebin.com/?gist=f2a99cc27c465ed51eb8159830836ca4. In it, each time I call update and bel creates a new element, a local network request is made for the base64 inlined image, even though it did not change.

And storing an element is useful sometimes, like for SVG icons in a separate file, which I now have to turn into functions, which is probably fine. This also seems to be related to an issue I was having when elements would not only re-render all the time, but not show up at all: https://github.com/shama/bel/issues/27.

[AutoSponge]

I think this is what you're trying to do: http://requirebin.com/?gist=07d10f19b4d8b3c229eab907a50df3b9

I'm using data-src to lazyload the img. Since the onload only fires once, it copies the data-src to src and the image appears. When subsequent img elements are created, they don't have a src, so no network request. When using morphdom to update, we can check whether or not the fromEl and toEl have matching data-src attributes. If they do, we don't need to update.

[arturi]

Thanks! That makes sense, yes. A bit cumbersome to use manually though. Maybe there should be some higher-lever abstraction that is aware of src attribute and is rewriting it to data-src or something like https://github.com/shama/bel/issues/26#issuecomment-228558252.

[timwis]

Hi @AutoSponge, thanks for the feedback. The use case that prompted the report originally was rendering a Leaflet map using an application framework that re-renders the page using morphdom any time a piece of state changes. Other libraries like d3 may do this as well, where calling their .init() function alters the DOM tree inside their container. Rather than re-initializing them at every re-render, it would be ideal to simply return a cached version of them.

Here's how I've implemented a map for my use case - it currently re-initializes if there's a state change though.

[AutoSponge]

I think there are two issues here, and I want to make sure I address both. First, my response before dealt with using a reference which causes a "flash" as an element leaves the document as it is moved from one DOM tree to another, then gets appended again. I think I was able to demonstrate that you don't want to do that. The non-destructive updates make sure that the onload only fires once.

However, this does not help the second issue which is that img elements load their src attribute aggressively which creates duplicate network requests every time the html method re-creates the img element. For that, I used one of the morphdom event hooks to prevent updating that element and used a simple data-* swapping technique.

For your map example, I altered your component slightly and it seems to work (but you know the code better):

module.exports = (coords) => {
  if (coords) {
    // If new coords or el hasn't been created already, create one
    // otherwise, cached el will be returned
    if (!isEqual(coords, cache.coords) || !cache.el) {
      cache.coords = coords
      if (cache.map) cache.map.remove()
      cache.el = html`<div class="map" id="map" onload=${onload} onunload=${onunload}></div>`
      return html`<div>${cache.el}</div>`
    }
    return html`<div>${cache.el.cloneNode(true)}</div>`
  } else {
    return html`<div class="map"></div>`
  }

The idea is that returning a clone of your cached element will copy all of the changes made by leafelet and cause no updates to the original element which would break the map. If this works for you, that's great, but it won't change my approach to the img issue.

[patrick-steele-idem]

I see the benefit of having a memoized HTML element, but the problem is that when morphdom sees that node it moves it into the real DOM and on the next render bel is putting that memoized node back into the new target DOM tree and that node can't exist in both places at once. I think what is missing here is the ability to clone a node before moving it into the real DOM so that the memoized node remains untouched. To make this happen I think we need to allow a new node to be returned by the onBeforeNodeAdded hook in morphdom and yo-yo would need to provide some ability to trigger a clone of memoized nodes using that hook.

Here's the relevant code in morphdom: https://github.com/patrick-steele-idem/morphdom/blob/3101545f268d4bf37291af9f99fa0e3c879f17f7/src/index.js#L513-L516

Thoughts? If that sounds good, would anyone be interested in submitting a PR?

EDIT: I think the key thing here is that the memoized DOM node can never be put in the real DOM because yo-yo/bel will just take it right back out during the next rerender.

[AutoSponge]

Just like I put the cloneNode in the render function above, I don't think this is a problem for morphdom to solve.

The html function of yo-yo/choo could check the parameters of the template literal and automatically clone a token that exists in the document (<node>.compareDocumentPosition(document) === 10)

[AutoSponge]

Btw, the cached node must be put into the DOM the first time because the 3rd-party side effects need to be cloned. But this may be the point to experiment with Vdom because the element could live in a Vdom tree.

[patrick-steele-idem]

I believe an improvement to morphdom could help here since it would be inefficient to clone the memoized DOM node every render. Instead, it would be better if the memoized DOM node would only be cloned if it needs to be added to the DOM tree, but otherwise the memoized DOM node would be used for diffing. If morphdom provided a hook that could be used to return a cloned node before adding it to the final DOM then that might be a nice enhancement.

[AutoSponge]

I see your point about efficiency.

If we choose to use a vdom object or custom element that morphdom understands to be equivalent to the actual DOM node (cachedNode#isSameNode(<node to compare>)), then the "cached" element in the DOM tree is a proxy implementing an equivalence check (faster than cloneNode). The check can refer to the cached node without being in the DOM tree.

The question then is, how to make this work with actual DOM? I'm going to test this out, but what if we used a custom element that implemented isSameNode and morphdom can always check isSameNode in morphEl and short-circuit if true is returned.

[patrick-steele-idem]

@AutoSponge diffing a virtual DOM node with a real DOM node definitely has promise. There would need to be the ability to upgrade the virtual DOM node to a real DOM node and that could also be done using the onBeforeNodeAdded hook if we allowed a DOM node to be returned from onBeforeNodeAdded.

[AutoSponge]

@patrick-steele-idem I'll try to make a POC for this using a custom component since that implementation is somewhat clear to me.

[AutoSponge]

@patrick-steele-idem I made this (quick and dirty) example: https://github.com/AutoSponge/morphdom-cached-node/blob/master/app.js so you can see what I mean.

I put the isSameNode check in two places but I didn't have time to run the tests yet. I'll try to do that tonight and confirm it works.