Proposal: invoke and await nested onbeforeremoves

[barneycarroll]

Description:

A sequential reading of the rewrite docs fills me with joy. The first time we encounter 'lifecycle hooks', it turns out we get very fine grain control, and we get it for any type of node!

But then it turns out that for onbeforeremove, "This hook is only called on the DOM element that loses its parentNode, but it does not get called in its child elements."

This is irritating because whereas the documentation up to this point has encouraged the idea of allocating lifecycle imperative code to whichever nodes it is most relevant to, this sudden exception forces the author to violate that principle and introduce their own boilerplate to account for something which could be done procedurally.

Steps to Reproduce:

Here's a bin with a component featuring animations. Click 'Edit' and then click 'Done', then dismiss the modal.

Expected:

On dismissal, I expected the modal to execute animations and wait for their resolution before being removed from the DOM.

The beauty of this code is that I don't have to push all my animation concerns into big centralised functions: the entry and exit animations are properties of the elements that they relate to. I can adjust them in place along with other concerns such as static styles, content, etc. The dom node is provided to me directly and the entry and exit animations are declared adjacently (useful for debugging!).

Actual:

Nothing happens, because technically these DOM nodes aren't being removed — one of their ancestors (the component root) is.

Here's the same thing, refactored to work around the shortcomings of onbeforeremove.

The ugliness of this code is that despite the implied potential of universal lifecycle methods, what is arguably the most common case for non-component lifecycle hooks — animation in and out — is impossible: so we revert to the dreaded 'fat controller' anti-pattern, where conceptually distinct concerns that we would otherwise expect to be grouped into their local sphere of reference are instead shoehorned into a top-level function that needs to do extra legwork to get the right references and set up the right conditions for async resolution, introducing indirection and brittleness. This looks silly, because I've split my entry and exit animation functions, which in my mind are intrinsically inter-linked, across large portions of orthogonal code. This pattern mixing feels wrong, and as a result I'm tempted to keep all animation code for a given component in its central lifecycle methods. At this point, the whole idea of per-element lifecycle hooks feels hollow.

Other minor points to consider:

• Deep querying the DOM from within component lifecycle methods feels like an anti-pattern. When this is necessary in Mithril v0, one at least benefits from the neat pattern of being able to bind element references to stateful props via config¹. This is considerably more verbose with Mithril v1's lifecycle hooks.
• It's tempting to read some kind of conceptual purity into the present implementation: the nodes in question aren't technically getting removed - only one node is getting removed; but that conception is inconsistent with eg oncreate, which technically creates leaf nodes first and appends up the tree — and yet oncreates trigger in source order, and allows leaf nodes in fresh trees to query their painted properties after their ancestor has been appended to the DOM. So the justification is internally inconsistent.

1. The prop + config centralised DOM reference pattern:

{
  controller : function(){
    this.el1= m.prop()
    this.el2 = m.prop()
  },

  view : ( { el1, el2 } ) => [
    m( '#el1', { config : el1 } ),
    m( '#el2', { config : el2 } )
  ]
}

[barneycarroll]

I make it sound like this caveat is sneakily hidden with the malicious purpose of disappointing me, but actually the very first mention of onbeforeremove in the table of lifecycle events specifically says:

This method is only triggered on the element that is detached from its parent DOM element, but not on its child elements

Please read the OP with an indignant outrage filter and colour me chastised 😅

[lhorie]

Theoretically, it would be nice if onbeforeremove could trigger in any vnode, but in reality, the edge cases and the difficulty of implementation make my brain hurt. For example, with those semantics, if a vnode is removed and one of its descendants has a onbeforeremove, then the parent will not be removed until the callback is called, and it seems somewhat unintuitive imho.

[barneycarroll]

Why is that unintuitive? You simply flatten a list of all subsequent methods, reduce them to a count that needs to be resolved, and wait for them all to execute. There's no new structures involved, conceptually. To put it in v0 terms: flatten the subtree, extract all onbefores, and m.sync those before the current node can be removed.

Am I missing an order of execution problem?

Well, I'm definitely missing something because as much as #1184 works on paper (introduces new tests that I think cover the problem, both pass), when I use that build in my problem case above, the component is still removed immediately.

But if you look at the patch in the render function, we already have a count check to wait for and I'm incrementing it synchronously before we run the resolution condition.

We can do the whole condition determination thing synchronously, right? I mean, I can do that proceduraly, without thinking, in application code in the second jsbin there in the OP (the only difficulty is in maintaining my DOM references). I parse the static subtree, extract the pertinent resolution conditions, and wait for them all to resolve.

What have I missed?

[lhorie]

Why is that unintuitive?

For example, let's say you have a bunch of widgets, one of which is a list of things, and that on deletion a list item slides out. If onbeforeremove is triggered upon ancestor removal, then the transition would fire for all list items on route change thus preventing an immediate route change

On the implementation side, the thing that makes my head hurt is that while onbeforeremove's callback is not called, the vdom tree is out of sync with the DOM, and I'm not entirely sure what would happen under the various diff scenarios if another couple of updates/redraws happened while in that state if the entire tree can be out of sync (rather than just the element that has the hook)

[barneycarroll]

I don't see the qualitative difference between these 2 problems:

1. During view reconciliation, a node is tagged for removal, and (maybe) the lifecycle is frozen while arbitrary author code resolves, at which point (certainly) the next view snapshot's lifecycle begins
2. During view reconciliation, an entire subtree of nodes is tagged for removal, at which point between 0 and all of them can opt to freeze the lifecycle until all such conditions are resolved.

The possibility of a done not being called or a state which supposedly exists now not being represented because the last state's teardown is async… These problems are already here. If you think these are inherently bad things, avoid this API; if you understand and value the bargain being made… The ability to describe subtle nested animation rules is not making that integral bargain qualitatively worse. Credibly, the difference is a quantitative thing based on designer judgment: am I using too much animation? Is this pretty effect worth the delay in persistent new data to screen?

[leeoniya]

in full agreement with @barneycarroll. once you've given the user [in]direct control over vdom state you always risk that the user will never call done() to either unfreeze a locked tree or let the lib finish a partial transaction. i've gone through all this reasoning myself when giving users the option to return Promises from will* hooks.

without this though, you end up forcing users to jump through a lot of hoops for trivial things like removal/unmounting/reordering animations. the other option is to provide a declarative interface for animations so you stay in control of the done()/Promise resolution (eg return fadeout duration). but this then restricts what can be done a great deal.

[spacejack]

As a user, yes, it would be nice to have onbeforeremove to fire when parent components are removed. If it's too problematic, an example workaround in the docs would be nice to have in that case.

[gilbert]

I likely don't fully understand the problem, but what if...

• onbeforeremove was recursive
• Returning false from its callback meant you stop the event from propagating downwards
• Changing route is like "returning false" by default?

[dead-claudia]

@mindeavor I think the biggest problem is that m.redraw() in the rewrite is fully synchronous. And async animations don't work well with that.

[lhorie]

I was thinking last night that maybe I could have nested onbeforeremoves be triggered, but for DOM removals to happen on a first-come-first-serve basis

for example, if parent schedules removal after 1 second, and child schedules removal after 2 seconds, parent is removed at 1 second

another example, if child is scheduled to be removed after 1 second and route changes, root is removed immediately because it's scheduled for synchronous dom element removal

so in the case of the hamburger menu that needs to close before route change, you can just add a delay in the root onbeforeremove to give the animation enough time to run, and if you don't want the delay, remove the onbeforeremove

The only issue is that you can't selectively tell children not to fire their onbeforeremove while a parent also has a delayed onbeforeremove, unless you do some shenanigans with setting flags yourself

[barneycarroll]

I wrote a little playground to explore the various edge cases around this, which might be helpful for trial and error: https://jsbin.com/zenoyi/edit?js,output

On reflection @lhorie I think your suggestion above is probably undesirable in the grand scheme of things, given the overall picture of what onbeforeremove can and cannot do. In conclusion I think the best path forward is to leave the current behaviour as it is, and write up some guidance as to what authors can reasonably expect from it.

The current behaviour is on-par with what's offered by Snabbdom and others, where the documented use case scenario is individual items being asynchronously removed from a dynamic list (in which case I probably wouldn't want every item on the list to perform outward animation when the containing component disappears — that's a semantically different thing in user terms). @leeoniya points here and elsewhere to the fact that a deeper awareness of deferred DOM patching is a bigger challenge than other competitive virtual DOM libs have achieved (that would be best served by a more holistic declarative API), and as you've said yourself @lhorie there are ambiguities of intent here which would be difficult to shoehorn into the current API, let alone implement.

---

…Here's my abortive attempt at implementing the compromise you're talking about above in user-land via a wrapper function for onbeforeremove: https://jsbin.com/zimiga/edit?js,output. It's naive, and broken, but it gives some indication of the complexity of assumptions necessary for implementation.

This exercise gave me a good deal of sympathy for #1086 (the new vnodes are so complex!) and #1229 (APIs with higher order flow control implications — event handlers' e.redraw, stream's HALT, onbeforemove's done — are painfully inconsistent and extremely hard to extend / intercept). All in all I really miss the redraw strategy API!

Having said that, it's now clear to me that the kind of behaviour I was originally calling for can be implemented in plugin land (I just need to patch m.render, hehe >:D), and shouldn't be expected from a baked-in lifecycle hook. I'll get plugging away at Exitable rewrite and report back when I've got something usable :)

[dead-claudia]

@barneycarroll I'll note that this might become possible if m.redraw becomes async (there's recent talk of this in #1166). In this event, I'd rather not nest them, though.

[dead-claudia]

Resurrecting this bug based on previous discussion in a separate, borderline-duplicate bug. Retargeting to v2.

[dead-claudia]

BTW, here's a copy of my proposal from over in that bug:

---

I've had a couple years to study highly async stuff further (including async DOM manipulation, which I've studied for quite a while, and other batched async updates), and here's what I'd reasonably expect:

1. All hooks are invoked at once, starting from the child and up to the parent.
2. Nothing is removed from a parent until its onbeforeremove and all its children's all resolve.
3. Once everything has been awaited (or if nothing needed awaited), then remove the tree.
4. This works holistically. If a child resolves before its parent, the child waits until the parent resolves before it's properly detached.
5. onremove hooks are fired in a separate pass after all onbeforeremove hooks are passed.
6. After those are all fired, the parent and all its children are unlinked. (The TreeWalker API could simplify this for us drastically.
7. Any future m.render calls on a tree would be blocked on existing onbeforeremove hooks. (This would also resolve this bug, which we need to do regardless of the outcome of this one.)

We could also add an extra options argument to m.redraw (related) to accept a resolve and reject argument, called on successful and failed application of a new tree. Although those wouldn't be applicable to m.route, it'd be applicable elsewhere (like in tests, where we could actually check for scheduled timings). We could also use those to provide hooks to make m.redraw() (async version) return a promise that resolves when finished and have m.redraw.sync() accept an extra options object to also expose this facility.

---

Needless to say, async isn't exactly easy nor simple, but that's mostly at the theoretical level. The implementation is much harder to plan than it is to write - it's hard to get right, but it's not super verbose to get right, at least not significantly more than what exists in this instance.

If we agree to this, I'll try to commit to it personally what I can.

---

Edit: Here's the comment after it:

As an alternative to my steps, you could swap steps 1-6 out to just do this, which could align with some's expectations at the cost of a little extra performance:

1. Do these steps on each child and await them.
2. Call onbeforeremove and await the result.
3. Call onremove.
4. Remove the node.

Either route we take needs to be documented explicitly so we can tell users what is supposed to happen.

[barneycarroll]

onbeforeremove (OBR) currently means "this node will be removed but its immediate context will persist". That last clause feels like an obtuse burden the first time it surprises you, but it's a useful guarantee and removing it opens up some awkward ambiguities.

You might start writing a tree with the expectation that descendant nodes' OBRs would be executed and allowed to hold up branch removal when an ancestor node is scheduled for removal, but in many other scenarios it's completely undesirable and difficult to differentiate from the distinct clause of the present mechanism.

Consider a leaderboard component consisting of a dynamic ordered list where items are expected to move about in relation to each other, and appear and disappear within the lifecycle of the leaderboard. oncreate means the entry is a new contender in the leaderboard. OBR means the contender has dropped off the leaderboard. These have specific meanings and any action within those hooks is liable to be intrinsically associated with those semantics. So in this context, the idea that removing the leaderboard from the UI should trigger the UI for all contenders leaving the board is undesirable, and if that were default behaviour, writing extra logic to determine when that is and isn't the case isn't intuitive.

On a more generic level, imagine an app making use of a consummate suite of generic UI components with incoming and outgoing animations: modals that slide down and back up out of view; form sub-sections that collapse on completion; media view panels that expand and shrink from a point of origin, a side bar that slides in and out from the left. Now consider the user navigating away from a page with a combination of such elements in view - back to the home page, or to their profile, etc. Do we expect all of these elements to attempt to simultaneously animate out? That sounds like a designer's nightmare.

In general the idea of procedurally triggering OBRs for all descendants of a removed ancestor violates the principle of higher order authority and isolated concerns. You'd have to introduce extra code to avoid the scenarios where it wasn't wanted, and in situations where you're making use of third party components that may be extra difficult.

By contrast, writing code to specifically instruct descendant nodes to animate out when on ancestor OBR is common stuff for UI developers. The relationships are explicit, the individual animations can be coordinated and sequenced according to holistic concerns, etc. It's not credible, coming from a perspective that's concerned with the practicalities of such things, to have Mithril do that sort of stuff 'automatically'.

[tivac]

It's not credible, coming from a perspective that's concerned with the practicalities of such things, to have Mithril do that sort of stuff 'automatically'.

Interesting to note the complete 180 in @barneycarroll's thinking on this over the intervening months.

I'm 👎 x 💯 on this, it seems like a huge source of unintended consequences. The current behavior is a reasonable compromise w/ an eye towards performance and given how few times it's come up since mithril hit 1.x I consider the anecdata to agree with me.

[dead-claudia]

@tivac I've 180'd too, but I know how to implement it now, and could do so within a few days at most. I also see the use for it, in that the few who've hit this and pointed it out don't really have any alternatives.

The current behavior is a reasonable compromise w/ an eye towards performance

And a naïve version of what we do would not substantially affect performance absent child onbeforeremove hooks, particularly if we go with this alternate route, which is also much simpler to implement. (I edited the original re-opening comment to add this part in.)

---

If we choose to decline this, we really need to better word in the docs what really happens.

This hook is only called on the DOM element that loses its parentNode, but it does not get called in its child elements.

This is technically correct, but we don't use that phrasing anywhere else. People tend to miss this, and it's written in a way that emphasizes the DOM, when we abstract and gloss over that pretty much everywhere else in the docs. It also is what has run people into problems with routing animations. It could probably be better worded as this:

This hook is only called on the top-most DOM element to be removed, not any child elements. If you use Mithril's router, note that because routing changes the component at the top of the tree, this is only called for the top-level component.

I do feel that the majority use case would be resolved if we just made routes like render-only functions - a common layout, for obvious reasons, wouldn't be removed and re-created in that instance, allowing onbeforeremove in child nodes to potentially be invoked as per usual.

[barneycarroll]

I wrote a component to allow explicit onbeforeremove dependency graphs. I'm planning on polishing it up along with some other view components as a toolkit for composing view concerns in Mithril — I'll post back for posterity when there's a published solution for this.

In the meantime, shall we use this ticket to earmark the documentation improvement suggestion?

[barneycarroll]

I'm maintaining a component called Waiter to abstract this functionality as part of my 'cascading components' repo.

The API works as follows. A higher order Waiters is passed a function to determine the sub-tree, and that function is provided with a link reference. Then lower order Waiter components are used to wrap immediate children whose OBR we want to be triggered by, and delay the removal of, the higher order component - these are given the link reference as an attribute of the same name. Straw man:

m(Waiter, link =>
  m(Waiter, {link}, X)
)

In this example, it's used to allow a lower order Modal component to fulfill OBR behaviour even though the UI logic requires a higher order component (Off) to sit above in the vtree.