Missing `ondestroy` event for deep removals

[mindplay-dk]

It appears the onremove event isn't triggered for children, when the parent is removed - I believe the following test demonstrates the problem:

https://github.com/mindplay-dk/picodom/commit/f108584890bdc936b44e8db5c071384bae3f3476

Looks like a bug? I'm unsure how to address it.

[jorgebucaran]

@zaceno What do you think?

[zaceno]

@JorgeBucaran

🤔 Hmm.. good question. I agree one would expect a lifecycle method called onremove to be called, even if the reason the element got removed was because a parent got removed.

I presume just changing the name to onremoveButOnlyWhenItsTheTopmostRemovedElement is out of the question? ;)

I don't know exactly how much performance and bundle size would suffer if we implemented support for it -- perhaps it wouldn't be too bad. But at the same time, I have yet to come across a use case where it was needed.

So perhaps just documenting this fact would be enough? onremove is a bit of an oddball anyway.

@mindplay-dk: Good find! Did you perchance have a use case that breaks because of it?)

[jorgebucaran]

@zaceno Can you clarify what you mean by oddball? I am just curious, I hope you don't mean onremove is buggy or wrongly implemented.

[zaceno]

@JorgeBucaran sure, no I don't mean it's wierd or buggy. I just mean I think it's somewhat uncommon to use it in the first place, that's all. The only use case I can think of, off hand is transitions. And in that case, if the parent goes away, it's not a problem that onremove is never called for the child.

I guess it's probably also useful for removing event listeners if some where bound in oncreate. But in that case, it should still be quite easy to handle if the parent goes away, with an onremove handler in the parent.

Now if that were common it would be an argument to move that complexity out of userland into picodom. But I don't know...

[mindplay-dk]

Events like oncreate and onremove are what enable integration with third-party JS components, which, for me, is one of the primary value proposals of picodom: integrating third-party plain JS components is really, really easy.

For example, using a third-party cropper or WYSIWYG editor, drag-and-drop lists, tree-controls, etc. there's usually going to be a destroy method of some sort that cleans up global event handlers, timers, etc. - which makes this feature pretty essential.

So I did a little research, and eventually discovered the solution to this in Snabbdom, which is two separate events:

1. remove, which gets triggered when an element gets directly removed from the DOM - this is what we have in pico now, and it's what you'd want to use for things like animation when a row gets removed from a table.

2. destroy, which gets called when an element is directly or indirectly being removed - we don't have this event in pico, but this is what you'd want for things like canceling timers, etc.

They're not interchangible, at all - they have completely different use-cases.

I think our life-cycle events are incomplete without this - IMO, we need to add a destroy event.

[jorgebucaran]

I have two questions:

1) What is the equivalent in React/Preact to destroy. 2) What are some good use cases for this event? I've never had the need for one and I've integrated third party tools like CodeMirror.

[mindplay-dk]

What is the equivalent in React/Preact to destroy.

As far as my understanding, there are no element-level life-cycle events in React - there's life-cycle events for Components only, as far as I know, and according to my coworker, when components get removed in cascades, there are unmount events for them, which is probably the closest equivalent.

What are some good use cases for this event? I've never had the need for one and I've integrated third party tools like CodeMirror.

How?

You'd be able to create your CodeMirror instance in oncreate, but I don't understand how you'd be able to destroy your CodeMirror instance again, since you can't count on the onremove event?

I suspect you may have successfully integrated it, but your CodeMirror instance never gets destroyed? Perhaps in your use-case, you didn't need to destroy it, which of course, then there's no problem.

Integrating most other third-party libraries, those generally have a destroy() function somewhere, which gives the library a chance to clean up: cancel any pending timers, removing global event listeners, removing a toolbar element it appended to the root body-element, etc.

I don't understand how you'd be able to integrate third-party libraries without this functionality, unless you just happen to have a use-case where cleaning up isn't necessary - but for any use-case that dynamically creates and destroys components, I don't understand how there's any alternative?

[mindplay-dk]

I've attempted an implementation, including a test-case - it partially works, but I can't figure out why the child-node goes missing for one of the two destroy-notifications...

[jorgebucaran]

@mindplay-dk Are you using keys?

[mindplay-dk]

@JorgeBucaran key-based operations only apply to the immediate parent - just like the onremove event. Why would that make any difference?

[jorgebucaran]

When I integrated codemirror with hyperapp, I used keys to make sure onremove was called for the element I used to save codemirror's instance.

But I think you have a point that I may have been missing something so, we should probably add a new ondestroy event that works similarly to snabbdom.

[mindplay-dk]

You're welcome to see if you can finish what I started - I've been working for most of the day, so I'm a bit low on steam for tonight, and I likely won't be able to put in more time until next weekend.

[jorgebucaran]

@mindplay-dk Thanks! Are you referring to this branch?

• https://github.com/mindplay-dk/picodom/tree/deep-onremove

[mindplay-dk]

Nope, this one here:

https://github.com/mindplay-dk/picodom/tree/destroy-event

[4lejandrito]

Hi!

As explained in https://github.com/hyperapp/hyperapp/issues/468, I experienced the same problem. Glad to see you guys are already working on a solution.

I'll be happy to help if needed.

[mindplay-dk]

@4lejandrito you're welcome to see if you can make the test pass :-)

[4lejandrito]

Possible fix implemented in hyperapp. See https://github.com/hyperapp/hyperapp/pull/470.

[jorgebucaran]

@mindplay-dk So, what I did in Hyperapp is rename the old onremove to onbeforeremove and the new ondestroy to onremove.

Before	Now
onremove	onbeforeremove
ondestroy	onremove

I suggest we proceed the same here.

@4lejandrito @zaceno

[mindplay-dk]

IMO that's unnecessarily verbose, hard to read (with the rere in the middle) and difficult to type.

Why not simply keep it consistent with Snabbdom?

[jorgebucaran]

What does snabbdom have to do with picodom? onbeforeremove is informative and consistent with how other DOM events are named:

• onafterprint
• onbeforeprint
• onbeforeunload

EDIT: If you care about consistency with other frameworks, fyi onbeforeremove is what mithril.js calls it.

[mindplay-dk]

Event hooks referring to two stages of the same event typically use the before/after prefixes - in this case, what I think we just established, is that removal and destruction are two distinct life-cycle events, more so than just two phases of the same event.

beforeunload for example occurs immediately before unload - they're two phases of the same event, and they're closely related, for example with beforeunload having the ability to cancel the unload event.

Contrast with remove and destroy which can occur seconds apart, and, as we've established, these events function completely independently of each other - for example, the remove event might never fire at all before a destroy event.

I think it's good they have distinct names, so that people understand that they are two completely distinct events with entirely different use-cases.

What does snabbdom have to do with picodom?

They exist in the same space and solve many of the same problems. There's nothing wrong with following something that's already established - just makes it easier for someone to move from one technology to another. And in this case it's hard to argue there's something wrong with the naming in Snabbdom, and I don't agree that the use of before in the proposed name is consistent with DOM naming.

Just my two cents :-)

[jorgebucaran]

This doesn't really bother me. I'll think about it! :)

[jorgebucaran]

@mindplay-dk I don't have a better name yet. 🤔

I see what you are saying, though, onbeforeremove is only called before onremove when the element is being directly removed, otherwise, when the element is indirectly removed as a result of removing the parent, only onremove in those elements will be called, even if they have an onbeforeremove.

With my choice I was trying to be informative, but not really being very good at it. Your suggestion is probably better, but I would prefer a name that was more descriptive.

• onremove
• ondestroy
• ondetach
• oncleanup
• onunload

[mindplay-dk]

In react terminology, components "mount" and "unmount".

"clean up" sounds more like what you might do in response to the element being destroyed, so I wouldn't use that for the event name - it's too loaded.

Honestly, I do think "remove" and "destroy" are very descriptive - because "remove" is the event triggered by the removal of a VNode from one update to the next, and "destroy" has a finality to it, it's what happens when everything else is over.

The potential problem with "remove" is it might imply to someone "remove" in the DOM sense of removeChild etc. - I'm hard pressed to think of a better term though....

[jorgebucaran]

@mindplay-dk You are right. What do you think about ondetach it has the same number of letters as oncreate/onupdate/onremove.

I think the issue I had with these two events could be addressed with better documentation. Onremove is called when elements are being directly removed, whereas ondestroy is called when the element is directly removed or when an ancestor (e.g., parent) is removed.

[mindplay-dk]

"detach" doesn't imply to me the finality - it sounds like it might get detached and then attached again somewhere else. (I'm not even 100% sure if you're suggesting "detach" instead of "remove" or "destroy".)

"removing" something from the DOM doesn't imply finality, as in DOM terms you can remove and then insert the same element again, as long as you have an element object reference.

"destroy" in OO terms correctly corresponds to the destruction of an element object once out of any scope.

[jorgebucaran]

Ah, the OO argument works for me, as that is exactly how you'd use ondestroy, as a destructor. 👍

[mindplay-dk]

It's good to have these discussions - terminology is super important; I find until you can put words on things, it's likely you don't yet understand (or don't agree) on the division of responsibilities to different units. I think by now we're both much clearer on what's what 😄

[mindplay-dk]

@JorgeBucaran this should be fixed in master now, so we can close this, right?

[jorgebucaran]

Yes! 👍