0.10.1 - error when find is called inside onunrender

[kouts]

Description:

When find is called inside the onunrender lifecycle event, produces an error when the instance is not rendered. This used to work fine in 0.9.13.

To resolve this I have added a guard inside the onunrender lifecycle event to check if the instance is rendered.

I think that this is either a) a breaking change b) a bug in 0.10.1 or c) a bug in 0.9.

Versions affected:

0.10.1

Platforms affected:

All Browsers

Reproduction:

Playground

[evs-chris]

I think this may be more an issue with lifecycle events and find. Technically, that findAll in 0.9.13 will never find anything because the instance is never rendered in unrender, and it should probably also throw there as it would in any other instance where you try to find DOM nodes in an un-rendered instance. 0.10.1 just added better rendered tracking.

This has actually cropped up for the other lifecycle events as well over the years - when x event fires is it before, during, or after x, and if during, what does that mean? Unrender is probably the most sensitive for this case exactly - the instance is about to be removed from the DOM and you need to do some cleanup before that happens. There should probably be an unmount or unrendering or something along those lines to handle that.

From the find and findAll side, I don't know that throwing is necessarily the best approach there, but them I'm not as big a fan of "fail early, fail hard" on things like that. I think a warning and just returning undefined and [], respectively, would suffice. If the instance is torn down, that would be a reason to throw.

[kouts]

Ok @evs-chris so this is basically more of an issue of the 0.9.x version if I understand correctly. I don't think another lifecycle event is needed, as long as unrender fires before DOM destruction I think it's ok to put any cleanup logic (unregistering custom events etc) in there. I always put cleanup logic in onunrender instead of onteardown, I hope that is the correct way to handle it.

[evs-chris]

According to #1256, which is the origin of lifecycle events, unrender is a bottom-up event, meaning that the instance will always already be unrendered before the event fires because the child components need to have their events fire first. There's not a good way to have unrender remain a bottom-up event and fire before the instance is no longer in the DOM, because the process of removing the instance from the DOM also triggers the child component events that make the process bottom-up.

I see three possible solutions to this:

1) Break unrender by making it top-down. This might make for some weirdness for components that listen to *.unrender for whatever reason. 2) Add another event that is top down and corresponds to just before unrender. Not sure that there's a good name for that - unrendering, beforeunrender, unmount are unpleasant in different ways. Another lifecycle event will add a bit more overhead to component-heavy templates. 3) Traverse the rendered template and fire unrender events before actually unrendering. This would leave unrender as bottom-up, but it also adds a good bit of code to the codebase and an extra full traversal of the VDOM during unrender, which is already not a light process.

Of those, I think 3 would be ideal behavior, but I don't think that 1 would really cause any issues in the wild and is the least complicated option.

[evs-chris]

After thinking on it a bit, adding an unrendering event as a top-down is non-breaking and doesn't add too much overhead. There are legit usecases for both sides of the unrender event, so I think this is the best solution. If you need to clean up externally managed DOM stuff from a component, unrendering is the event you'll want from 0.10.2. Thanks for the report!

[kouts]

Thank you @evs-chris , so unrender event must not be used for cleanup?

[evs-chris]

unrender can be used for component cleanup, like dropping listeners and cancelling observers, but if you need to do DOM stuff, like tear down integrations with third party libs that were set up in render, then unrendering is the place for that. Of course, you can also do event/observer cleanup there too.

[kouts]

Ok @evs-chris I think I got it, thank you once again.