🐛 Cleans up template directives memory

[ekwoka]

Eagerly cleans up clones from x-if and x-for template directives.

Addresses: #4299 #4287 #4231 #1730 #3980 and others...

The Problem

Basically, when x-if and x-for cleaned up their cloned elements, they mainly left it to the mutation observer to but this was not always correctly handling the tree when it came to these elements. Resulting in primarily non-destructive errors, but also in a significant memory leak.

The x-for would not dequeue any scheduled jobs, so already triggered effects would run even if x-for removed the element, while x-if would dequeue existing jobs, it would actually prevent many cleanups on nested elements from actually being run.

This mainly became an issue when x-if and x-templates were nested (in each other or themselves) as the duplication of elements and effects could escalate.

Due to how the mutation observer worked, it would not see the removed nested children when cleaning, as the nested tags would remove the element (without cleaning it) when they were cleaned, leaving the child completely detached and unobserved.

The Change

Directly in the relevant directive, when the element is cleaned, instead of deferring handling to the mutation observer, immediately remove and clean the tree (similar to how init was not deferred to the mutation observer).

This also includes destroyTree taking on dequeuing of active jobs as it destroys the tree.

Breakages?

None that I found or could think of. There shouldn't be any risk to actually cleaning the tree when it's removed, as opposed to some time a few milliseconds in the future.

[calebporzio]

This is great @ekwoka. I'm slightly hesitant because I feel like leaving it to the mutation observer was important for some deep morphing bug or something. However, I ran all the Livewire tests against this PR and everything seems fine.

I'm going to move forward with it because it's obv such an improvement, but let's keep an eye on it and we might have to revert and find a different strategy if some of those deep issues resurface.

Thanks!

[ekwoka]

I tried to think real hard about the risks, but couldn't imagine any that would be reasonable.

If anything was counting on the cleanup to not happen immediately, I can't imagine how it would work that wouldn't be at risk of race conditions in the old version.

It's possible anything that does/did depend on that may have been changed, or has a safer way to be implemented. If it looks like this causes an issue in LiveWire (that doesn't present in normal Alpine), I can hop over there and help out.

[mgschoen]

@ekwoka I believe I found a regression introduced with this PR. I checked the git history pre-merge of this PR and the issue did not exist before.

Consider the following situation:

<template x-if="isVisible">
    <div>
        <template x-teleport="#somewhere">
            <div x-data="{ destroy() { console.log('destroy'); } }"></div>
        </template
    </div>
</template>

When isVisible changes to false, the destroy() method of x-teleport's content is not called. Neither are effects and watchers removed. Same applies if x-teleport gets removed as child of x-for.

I believe this is because x-teleport also relies on the MutationObserver to clean up its content. Due to the usage of mutateDom() in the cleanups of x-if and x-for, the removal is not observed.

Not sure what's the best solution though. Make x-teleport explicitly destroy its tree as well? Or rather refrain from using mutateDom()?

I'd be happy if you could look into this!

[ekwoka]

Ah, I had tested teleport and not found anything, but that does seem like an issue

How did you end up using this build, btw? It's prerelease.

[mgschoen]

Built Alpine locally and pulled it into our project for testing purposes :)