DOM driver elements() sync emissions in a loop

[staltz]

Strongly related to #592 and a couple different PRs I've been doing to fix that issue, I found another issue, this one is mostly related to the DOM driver, but also related to cycle/run. This issue is also a race condition, but it's not cross-driver. In fact there is only one driver involved.

The interesting thing of this bug is that it only happens in cycle/dom if we fix cycle/run. So basically it's a trade of bugs. Because cycle/run is buggy, cycle/dom doesn't show this bug, but fundamentally it's there.

Code to reproduce the issue:

function main(sources) {
  const elem$ = sources.DOM.select(':root').elements();
  const vnode$ = elem$.map(elem => h('div.bar', 'left=' + elem.offsetLeft));
  return {
    DOM: vnode$,
  };
}

The important thing in that code above is that the DOM sink depends on the DOM source (elements()) and vice-versa (the elements() changes every time the DOM sink changes). It looks like an infinite loop, and indeed it is. Except, due to a bug in cycle/run, the infinite loop gladly does not happen.

Here's the bug in cycle/run, replicateMany:

  sinkNames.forEach(name => {
    const listener = sinkProxies[name];
    const next = (x: any) => {
      listener._n(x);
    };
    const error = (err: any) => {
      logToConsoleError(err);
      listener._e(err);
    };
    // (A) BUFFERS CONSUMED
    buffers[name]._n.forEach(next);
    buffers[name]._e.forEach(error);
    // (B) REPLICATORS UPDATED
    replicators[name].next = next;
    replicators[name].error = error;
    replicators[name]._n = next;
    replicators[name]._e = error;
  });

In step (A), we consume everything from the buffers array. However, when that is happening forEach(next) will actually add more stuff to the buffers. And those stuff should have been processed and fed to next but they weren't. This is what prevents the infinite sync loop from happening.

If we make this change (which makes sense):

  sinkNames.forEach(name => {
    const listener = sinkProxies[name];
    const next = (x: any) => {
      listener._n(x);
    };
    const error = (err: any) => {
      logToConsoleError(err);
      listener._e(err);
    };
    // (B) REPLICATORS UPDATED
    replicators[name].next = next;
    replicators[name].error = error;
    replicators[name]._n = next;
    replicators[name]._e = error;
    // (A) BUFFERS CONSUMED
    buffers[name]._n.forEach(next);
    buffers[name]._e.forEach(error);
  });

then cycle/run behaves correctly, but the cycle/dom example will run into an infinite sync loop.

Versions of packages used:

cycle/run: 3.1
cycle/dom: 18.3

Tentative solution:

All these DOM sink emissions will be vnodes that look all the same, even though they are not exactly the same vnode object, so they cause a snabbdom patch and therefore a new emission of DOM element, which we can't do simple === equality check in a dropRepeats because in the normal case we are mutating the DOM element, hence its === check will be true even though its contents are different.

We wouldn't have this problem if objects in JavaScript would be immutable and have content-based hash, but that's unfortunately not the case. I'm not yet sure what should be the solution, but putting a vnode$.dropRepeats(somethingSmart) inside the DOM driver is one idea, and I'm not sure what should be somethingSmart and how to keep that efficient.

I also have a feeling other frameworks have had this exact same issue, would be interesting to see their solutions.

Ideas? @jvanbruegge @whitecolor @TylorS ?

[jvanbruegge]

Actively work on pull which fixes this issue?

[staltz]

Yes! I was going to mention that, and forgot. It's a real alternative, and makes sense too if elements represents a pullable-value.

But also we have to approach the pull version of the DOM driver carefully, because it might not be the solution to everything. For instance, it comes with the tradeoff that snabbdom patches (even if they are light) will happen redundantly on every animation frame.

[jvanbruegge]

Not with the update integers optimization

[staltz]

Are you sure? The update integers optimization will work with e.g. onionify when vdomS = stateS.map(render), but when vdomS = elementS.map(render) then the update integer inside elementS depends on the snabbdom patch rendering which depends on vdomS update integer.

---

Here's another idea: apply memoization on hyperscript so that h('div.bar', 'left=' + elem.offsetLeft) is a function call that returns the same memoized object. Did I just reinvent thunks?

[wclr]

Well, I've seen such thing long before when working with elements I just put .elements().compose(dropRepeats()) and this fixed the issue for me, do you think it is not enough?

[wclr]

From docs:

DOMSource.elements() returns a stream of the DOM element(s) matched by the selectors in the DOMSource. Also, DOMSource.select(':root').elements() returns a stream of DOM element corresponding to the root (or container) of the app on the DOM.

To me elements() was always strange thing. First, it is not clear, what the stream of the DOM elements means, what is the law when elements are emitted? Are they added to DOM, or modified, patched? What should it be by intention?

Form DOM driver code it is just directly derived stream from this._rootElement$.map(el => elementFinder.call(el));, well then I don't understand really why you consider this to be a bug.

You send something to sink it will still go to _rootElement$ stream then you find element in this stream value and send it to source elements.

I'm I missing something?

[staltz]

elements() should be understood as a stream of DOM elements emitted when the corresponding DOM element has had its contents changed (or initialized). When a redundant patch of VNode h('span', 'Hello') on an element <span>Hello</span> is applied, the DOM element will not have any of its contents changed (snabbdom won't touch the DOM in any way) during snabbdom patch, and therefore we should not be expecting a new emission of rootElement$, however, the emission happens.

One solution would be to change snabbdom so that patch() returns a boolean indicating whether the DOM was touched, but patch already returns something: patch(prevVNode, nextVNode): VNode, so that's a problem, would require a big rework of snabbdom. Another solution would be to customize the modules and domAPI passed to snabbdom init: init(modules, domAPI) so that we have boolean flags internally in modules and domAPI which will indicate the DOM being touched.

[wclr]

So you fix of changing the order of assigning replicators and buffers is correct, and this issue that appeared after refers to DOM driver because it emits things that it should not by formal intention.

I don't really get deep into snabbdom to understand how it works.

[geovanisouza92]

Make .elements() emit based on MutationObserver isn't an option?

[brucou]

@geovanisouza92 yep. was thinking that. Looks like a source of truth issue. If the source of truth is the DOM and the DOM hasn't changed, then elements should not emit. i.,e. the graph should be VTree -> DOM -> elements instead of VTree -> elements. Haven't looked up implementation of elements so just thinking out loud and hypothetizing here.

Other possibility I can think of is that you can always turn an infinite sync loop into an infinite async loop by using a delay somewhere in the loop. At least that avoids monopolizing the thread. The tradeoff is that may also create other synchronization problems cross drivers, if those drivers have some dependency relation with the DOM - which they generally have... Damn, so maybe delay all drivers by the same amount. That awefully looks like the former implementation of the cycle loop with .delay(1) of I think maybe two years ago though.

[geovanisouza92]

I'm just a newbie on Streams thing, but I don't think that delaying things here and there is good on the long term. Look, there's a buffering logic already.

---

It really seems weird that MainDOMSource' rootElement$ emits from VNode instead of DOM itself. What would be the result if an external script change something inside the app vtree$?

---

Maybe it doesn't need to be something complicated:

let observer: MutationObserver;

const mutationConfirmed$ = xs.create({
  start (listener) {
    observer = new MutationObserver(listener.next);
  },
  stop () {
    observer.disconnect();
  }
});

rootElement$.compose(sampleCombine(mutationConfirmed$)) // ...


// After patch()
observer.observe(rootElement)

(just a suggestion, of course)

[wclr]

I think there should be no need in involving MutatinObserver, snabbdom already knows if it has modified something or not, no need to involve additional resources.

[staltz]

Actually I liked the MutationObserver suggestion, thanks @geovanisouza92. I did that in PR #719, would be good to get reviews on it.

@whitecolor snabbdom "knows" it but snabbdom doesn't indicate to us this knowledge. It's possible, but involves serious changes into snabbdom (a library used by others, not just Cycle.js). It's also possible to fork snabbdom (we've almost done that, more than once) but MutationObserver seems like it is made for the use case we want here: to know whether or not something was mutated on the DOM.

[wclr]

Actually, I didn't even know what is MutationObserver and how it works. Well if it works fine without spending many resources. You better publish dom rc. (I checked latest run rc with microtask it seems to work ok)

[staltz]

I was surprised that it was made as a faster mechanism than existing mechanisms: https://hacks.mozilla.org/2012/05/dom-mutationobserver-reacting-to-dom-changes-without-killing-browser-performance/ and primary use case is for framework authors:

I don’t expect most JS hackers are going to run out right now and start adding mutation observers to their code. Probably the biggest audience for this new api are the people that write JS frameworks, mainly to solve problems and create interactions they could not have done previously, or at least not with reasonable performance.

:)

[staltz]

Remember to test if #410 is still an issue, after this issue is resolved.

[staltz]

New versions of cycle/run and cycle/dom will come out for this issue, soon