Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Cleans up template directives memory #4300

Merged
merged 7 commits into from
Jul 16, 2024

Conversation

ekwoka
Copy link
Contributor

@ekwoka ekwoka commented Jul 12, 2024

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.

@ekwoka ekwoka changed the title 🐛 Cleans up template directives 🐛 Cleans up template directives memory Jul 12, 2024
@calebporzio
Copy link
Collaborator

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!

@calebporzio calebporzio merged commit 6ac9782 into alpinejs:main Jul 16, 2024
1 check passed
@ekwoka
Copy link
Contributor Author

ekwoka commented Jul 16, 2024

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
Copy link
Contributor

mgschoen commented Aug 1, 2024

@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
Copy link
Contributor Author

ekwoka commented Aug 1, 2024

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
Copy link
Contributor

mgschoen commented Aug 1, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants