:bug: Cleans up template directives memory (#4300)

* :test_tube: Adds Failing Tests for Memory Cleanup

* :white_check_mark: Cleans up x-if eagerly

* :test_tube: corrects x-for test

failed for the wrong reasons

* :white_check_mark: Cleans x-for tree

* :recycle: Moves Effect dequeuing to Element Cleanup

* formatting

* formatting

---------

Co-authored-by: Caleb Porzio <calebporzio@gmail.com>

packages/alpinejs/src/directives/x-for.js

@@ -2,10 +2,9 @@ import { addScopeToNode } from '../scope'
 import { evaluateLater } from '../evaluator'
 import { directive } from '../directives'
 import { reactive } from '../reactivity'
-import { initTree } from '../lifecycle'
+import { initTree, destroyTree } from '../lifecycle'
 import { mutateDom } from '../mutation'
 import { warn } from '../utils/warn'
-import { dequeueJob } from '../scheduler'
 import { skipDuringClone } from '../clone'
 
 directive('for', (el, { expression }, { effect, cleanup }) => {
@@ -23,7 +22,13 @@ directive('for', (el, { expression }, { effect, cleanup }) => {
     effect(() => loop(el, iteratorNames, evaluateItems, evaluateKey))
 
     cleanup(() => {
-        Object.values(el._x_lookup).forEach(el => el.remove())
+        Object.values(el._x_lookup).forEach(el =>
+            mutateDom(() => {
+                destroyTree(el)
+
+                el.remove()
+            }
+        ))
 
         delete el._x_prevKeys
         delete el._x_lookup
@@ -139,19 +144,18 @@ function loop(el, iteratorNames, evaluateItems, evaluateKey) {
         // for browser performance.
 
         // We'll remove all the nodes that need to be removed,
-        // letting the mutation observer pick them up and
-        // clean up any side effects they had.
+        // and clean up any side effects they had.
         for (let i = 0; i < removes.length; i++) {
             let key = removes[i]
 
-            // Remove any queued effects that might run after the DOM node has been removed.
-            if (!! lookup[key]._x_effects) {
-                lookup[key]._x_effects.forEach(dequeueJob)
-            }
+            if (! (key in lookup)) continue
 
-            lookup[key].remove()
+            mutateDom(() => {
+                destroyTree(lookup[key])
+
+                lookup[key].remove()
+            })
 
-            lookup[key] = null
             delete lookup[key]
         }
 

packages/alpinejs/src/directives/x-if.js

@@ -1,10 +1,8 @@
 import { evaluateLater } from '../evaluator'
 import { addScopeToNode } from '../scope'
 import { directive } from '../directives'
-import { initTree } from '../lifecycle'
+import { initTree, destroyTree } from '../lifecycle'
 import { mutateDom } from '../mutation'
-import { walk } from "../utils/walk"
-import { dequeueJob } from '../scheduler'
 import { warn } from "../utils/warn"
 import { skipDuringClone } from '../clone'
 
@@ -30,13 +28,11 @@ directive('if', (el, { expression }, { effect, cleanup }) => {
         el._x_currentIfEl = clone
 
         el._x_undoIf = () => {
-            walk(clone, (node) => {
-                if (!!node._x_effects) {
-                    node._x_effects.forEach(dequeueJob)
-                }
-            })
+            mutateDom(() => {
+                destroyTree(clone)
 
-            clone.remove();
+                clone.remove()
+            })
 
             delete el._x_currentIfEl
         }

packages/alpinejs/src/lifecycle.js

@@ -98,8 +98,8 @@ export function initTree(el, walker = walk, intercept = () => {}) {
 
 export function destroyTree(root, walker = walk) {
     walker(root, el => {
-        cleanupAttributes(el)
         cleanupElement(el)
+        cleanupAttributes(el)
     })
 }
 

packages/alpinejs/src/mutation.js

@@ -1,3 +1,4 @@
+import { dequeueJob } from "./scheduler";
 let onAttributeAddeds = []
 let onElRemoveds = []
 let onElAddeds = []
@@ -40,9 +41,9 @@ export function cleanupAttributes(el, names) {
 }
 
 export function cleanupElement(el) {
-    if (el._x_cleanups) {
-        while (el._x_cleanups.length) el._x_cleanups.pop()()
-    }
+    el._x_effects?.forEach(dequeueJob)
+
+    while (el._x_cleanups?.length) el._x_cleanups.pop()()
 }
 
 let observer = new MutationObserver(onMutate)

tests/cypress/integration/directives/x-for.spec.js

@@ -605,3 +605,40 @@ test('x-for throws descriptive error when key is undefined',
     ({ get }) => {},
     true
 )
+
+// If x-for removes a child, all cleanups in the tree should be handled.
+test('x-for eagerly cleans tree',
+    html`
+        <div x-data="{ show: 0, counts: [0,0,0], items: [0,1,2] }">
+            <button
+                id="toggle"
+                @click="show^=true"
+                x-text="counts.reduce((a,b)=>a+b)">
+                Toggle
+            </button>
+            <button id="remove" @click="items.pop()">Remove</button>
+            <template x-for="num in items" :key="num">
+                <div>
+                <template x-for="n in show">
+                    <p x-effect="if (show) counts[num]++">hello</p>
+                </template>
+                </div>
+            </template>
+        </div>
+    `,
+    ({ get }) => {
+        get('#toggle').should(haveText('0'))
+        get('#toggle').click()
+        get('#toggle').should(haveText('3'))
+        get('#toggle').click()
+        get('#toggle').should(haveText('3'))
+        get('#toggle').click()
+        get('#toggle').should(haveText('6'))
+        get('#remove').click()
+        get('#toggle').should(haveText('6'))
+        get('#toggle').click()
+        get('#toggle').should(haveText('6'))
+        get('#toggle').click()
+        get('#toggle').should(haveText('8'))
+    }
+)

tests/cypress/integration/directives/x-if.spec.js

@@ -110,3 +110,40 @@ test('x-if removed dom does not attempt skipping already-processed reactive effe
         get('div#div-also-not-editing').should(exist())
     }
 )
+
+// If x-if evaluates to false, all cleanups in the tree should be handled.
+test('x-if eagerly cleans tree',
+    html`
+        <div x-data="{ show: false, count: 0 }">
+            <button @click="show^=true" x-text="count">Toggle</button>
+            <template x-if="show">
+                <div>
+                <template x-if="true">
+                    <p x-effect="if (show) count++">
+                    hello
+                    </p>
+                </template>
+                </div>
+            </template>
+        </div>
+    `,
+    ({ get }) => {
+        get('button').should(haveText('0'))
+        get('button').click()
+        get('button').should(haveText('1'))
+        get('button').click()
+        get('button').should(haveText('1'))
+        get('button').click()
+        get('button').should(haveText('2'))
+        get('button').click()
+        get('button').should(haveText('2'))
+        get('button').click()
+        get('button').should(haveText('3'))
+        get('button').click()
+        get('button').should(haveText('3'))
+        get('button').click()
+        get('button').should(haveText('4'))
+        get('button').click()
+        get('button').should(haveText('4'))
+    }
+)