Await resolution of subtree's onbeforeremoves before removal

[barneycarroll]

As described in #1182

1. Executes all onbeforeremoves
2. Awaits the resolution of each before proceeding with removal

[pygy]

Wouldn't this worsen the already less than ideal remove perf?

[barneycarroll]

Yeah, that's a point! I think the practical edge cases of why this doesn't "just work" the way I described in #1182 are only just sinking in: without time travel, you need every node liable to removal to reiterate through its entire subtree to check for removal conditions.

With the current patch, in order to meet my aims, that would require the author to declare an onbeforeremove method with a done invocation for every node that may be subject to removal. And if you follow this implication to its logical conclusion, with the API as it stands, that means a systematically ambiguous introduction of seemingly pointless API cruft whenever the author believes subtrees may require exit animation¹.

@pygy you raise the good point that this introduces potential for authors to significantly screw the potential preformance of the draw lifecycle, because as I've just worked out, you introduce the potential for any number of concentric re-traversals of the subtree in order to determine potential removal dependencies. IMO this isn't (currently) a serious concern because traversing a vnode tree is both trivial / orthogonal to the concerns of modern animation techniques.

I'd just like to take a break in this wall of text to insist that a decent API for outgoing animations is an incredibly worthwhile thing. It was part and parcel of web apps before MVC took over the world and we are throwing user gratification and application possibility space to the wind if we say that this isn't worthy of our concern even in the face of implementation and API design trickiness.

So let's look at our options:

1. Accept the fundamental assumptions in my implementation (author must explicitly annotate removable nodes to allow for subtree crawling upon removal scheduling).
   1. And just accept my patch with current author-land API overload ¹
   2. Reformulate the API for onbeforeremove to make the completion contract less obtuse. I think this is has considerable generic benefits orthogonal to this particular concern². Why not allow the return of a thenable rather than introduce a unique situation of a mystery second argument?
2. Procedural concentric traversal of the subtree in the eventuality of a node removal (exponentially expensive)
   3.…?

---

1. Author-land implementation code:

// I *know* I'm subject to removal 
export default const Modal = {
  onbeforeremove : ( {}, done ) =>
    done(),

  view : ( { children } ) =>
    m( '.Modal',
      m( '.barrier', {
        onbeforeremove : x
      } ),

      m( '.content', {
        onbeforeremove : x
      },
        children
      )
    )
}

1. I'm kinda surprised by the inconsistency within the v1 API with regards to standard contract expression. Promises have been ditched; Props no longer then, which is fine of itself; onbeforeupdate has wildly different implications depending on whether or not it returns false; onbeforeremove mandates invocation of a second argument — surprising given that my prototype for deferred exit hooks, Exitable, is already more consistent with onbeforeupdate in relying on return values, which works extremely intuitively with arrow functions and spurns the Node-style 'call argument November depending on concerns', which is totally inconsistent with the functional approach mandated by the aforementioned. Why can't we just return a thenable? If the answer to that is 'because Mithril doesn't do then anymore, so consistency demands…' then I think that's pretty poor given the existing inconsistency previously described and the fact that Exitable had already proven a decent API for these concerns and @JAForbes (who I see as the key influence in bringing 'streaming' props to Mithril's purview, along with the individual API methods) has explained at great effort that the value in adopting these new verbs and contracts is consistency between orthogonal APIs.

[pygy]

@barneycarroll check this out :-) AFAICT you can tick the second item of your list.

Somehow github won't let me send you a PR... I've rebased this on top of the current head, feel free to pull it from my fork to push it in yours :-)

[pygy]

Ooooh... I hadn't seen the gitter discussion... in light of this, my addition may be misguided.

The way you implemented it, if the first hook calls done() synchronously, the removal will occur immediately, before the other hooks are processed (since at that point expected === called === 1). It may actually be a feature (I'd argue its a hack, personally).

Otherwise, we may rely on calling done(true) synchronoulsy to skip the children of the elements involved (but not its siblings). That could be implemented cleanly on top of my patch.

Edit: actulally, no, I need to sleep... That won't help. You'd need to build a tree of callbacks first, then traverse it and optionally skipping the children using either done(true) or more simply the return value as suggested by @mindeavor in #1182.

[barneycarroll]

After playing around with this patterns use case and implementation strategies on and off for a while, I think it's reasonable to close this:

• As @lhorie said early on, the feature could easily be considered a bug in that a higher order component should arguably be totally explicit in its own onbeforeremove definition, implicitly independent of the concerns of lower order components further down the view graph.
• API extension is required to address ambiguity of intent WRT race conditions etc.
• Any implementation of the desired feature requires a significant amount of performance-negative logic whenever a node is removed, which would disproportionately impact common scenarios to cater for exceptions.

I've opened #1486 to deal with the Promise proposal which arose as a tangent.