feat: implement cleanup on destroy for async computed

Author: Varixo

packages/qwik/src/core/client/vnode-diff.ts

@@ -50,7 +50,7 @@ import { hasClassAttr } from '../shared/utils/scoped-styles';
 import { serializeAttribute } from '../shared/utils/styles';
 import { isArray, type ValueOrPromise } from '../shared/utils/types';
 import { trackSignalAndAssignHost } from '../use/use-core';
-import { TaskFlags, cleanupTask, isTask } from '../use/use-task';
+import { TaskFlags, isTask } from '../use/use-task';
 import type { DomContainer } from './dom-container';
 import { VNodeFlags, type ClientAttrs, type ClientContainer } from './types';
 import { mapApp_findIndx, mapArray_set } from './util-mapArray';
@@ -81,6 +81,9 @@ import {
 } from './vnode';
 import type { ElementVNode, TextVNode, VNode, VirtualVNode } from './vnode-impl';
 import { getAttributeNamespace, getNewElementNamespaceData } from './vnode-namespace';
+import { cleanupDestroyable, isDestroyable } from '../use/utils/destroyable';
+import { SignalImpl } from '../reactive-primitives/impl/signal-impl';
+import { isStore } from '../reactive-primitives/impl/store';
 
 export const vnode_diff = (
   container: ClientContainer,
@@ -1527,29 +1530,33 @@ export function cleanup(container: ClientContainer, vNode: VNode) {
     if (type & VNodeFlags.ELEMENT_OR_VIRTUAL_MASK) {
       clearAllEffects(container, vCursor);
       markVNodeAsDeleted(vCursor);
-      // Only elements and virtual nodes need to be traversed for children
-      if (type & VNodeFlags.Virtual) {
+
+      const isComponent =
+        type & VNodeFlags.Virtual &&
+        (vCursor as VirtualVNode).getProp<OnRenderFn<any> | null>(OnRenderProp, null) !== null;
+      if (isComponent) {
+        // cleanup q:seq content
         const seq = container.getHostProp<Array<any>>(vCursor as VirtualVNode, ELEMENT_SEQ);
         if (seq) {
           for (let i = 0; i < seq.length; i++) {
             const obj = seq[i];
             if (isTask(obj)) {
-              const task = obj;
-              clearAllEffects(container, task);
-              if (task.$flags$ & TaskFlags.VISIBLE_TASK) {
-                container.$scheduler$(ChoreType.CLEANUP_VISIBLE, task);
-              } else {
-                cleanupTask(task);
+              clearAllEffects(container, obj);
+              if (obj.$flags$ & TaskFlags.VISIBLE_TASK) {
+                container.$scheduler$(ChoreType.CLEANUP_VISIBLE, obj);
+                // don't call cleanupDestroyable yet, do it by the scheduler
+                continue;
               }
+            } else if (obj instanceof SignalImpl || isStore(obj)) {
+              clearAllEffects(container, obj);
+            }
+
+            if (isDestroyable(obj)) {
+              cleanupDestroyable(obj);
             }
           }
         }
-      }
 
-      const isComponent =
-        type & VNodeFlags.Virtual &&
-        (vCursor as VirtualVNode).getProp<OnRenderFn<any> | null>(OnRenderProp, null) !== null;
-      if (isComponent) {
         // SPECIAL CASE: If we are a component, we need to descend into the projected content and release the content.
         const attrs = vnode_getProps(vCursor as VirtualVNode);
         for (let i = 0; i < attrs.length; i = i + 2) {

packages/qwik/src/core/shared/scheduler.ts

@@ -98,14 +98,7 @@ import {
 import { scheduleEffects } from '../reactive-primitives/utils';
 import { type ISsrNode, type SSRContainer } from '../ssr/ssr-types';
 import { runResource, type ResourceDescriptor } from '../use/use-resource';
-import {
-  Task,
-  TaskFlags,
-  cleanupTask,
-  runTask,
-  type DescriptorBase,
-  type TaskFn,
-} from '../use/use-task';
+import { Task, TaskFlags, runTask, type DescriptorBase, type TaskFn } from '../use/use-task';
 import { executeComponent } from './component-execution';
 import type { OnRenderFn } from './component.public';
 import type { Props } from './jsx/jsx-runtime';
@@ -127,6 +120,7 @@ import { isSsrNode } from '../reactive-primitives/subscriber';
 import { logWarn } from './utils/log';
 import type { ElementVNode, VirtualVNode } from '../client/vnode-impl';
 import { ChoreArray, choreComparator } from '../client/chore-array';
+import { cleanupDestroyable } from '../use/utils/destroyable';
 
 // Turn this on to get debug output of what the scheduler is doing.
 const DEBUG: boolean = false;
@@ -700,7 +694,7 @@ This is often caused by modifying a signal in an already rendered component duri
       case ChoreType.CLEANUP_VISIBLE:
         {
           const task = chore.$payload$ as Task<TaskFn, TaskFn>;
-          cleanupTask(task);
+          cleanupDestroyable(task);
         }
         break;
       case ChoreType.NODE_DIFF:

packages/qwik/src/core/tests/use-async-computed.spec.tsx

@@ -307,4 +307,45 @@ describe.each([
       (globalThis as any).log = undefined;
     });
   });
+
+  describe('cleanup', () => {
+    it('should run cleanup on destroy', async () => {
+      (globalThis as any).log = [];
+
+      const Child = component$(() => {
+        const asyncValue = useAsyncComputed$(({ cleanup }) => {
+          cleanup(() => {
+            (globalThis as any).log.push('cleanup');
+          });
+          return Promise.resolve(1);
+        });
+        return <div>{asyncValue.value}</div>;
+      });
+
+      const Counter = component$(() => {
+        const toggle = useSignal(true);
+
+        return (
+          <>
+            <button onClick$={() => (toggle.value = !toggle.value)}></button>
+            {toggle.value && <Child />}
+          </>
+        );
+      });
+      const { container } = await render(<Counter />, { debug });
+      // on server its called after render
+      // on client it is not called yet
+      expect((globalThis as any).log).toEqual(render === ssrRenderToDom ? ['cleanup'] : []);
+      await trigger(container.element, 'button', 'click');
+      // on server after resuming cleanup is not called yet
+      // on client it is called as usual
+      expect((globalThis as any).log).toEqual(
+        render === ssrRenderToDom ? ['cleanup'] : ['cleanup']
+      );
+      await trigger(container.element, 'button', 'click'); //show
+      await trigger(container.element, 'button', 'click'); //hide
+      // on server and client cleanup called again
+      expect((globalThis as any).log).toEqual(['cleanup', 'cleanup']);
+    });
+  });
 });

packages/qwik/src/core/use/use-resource.ts

@@ -4,7 +4,7 @@ import { isServerPlatform } from '../shared/platform/platform';
 import { assertQrl } from '../shared/qrl/qrl-utils';
 import { type QRL } from '../shared/qrl/qrl.public';
 import { invoke, newInvokeContext, untrack, useBindInvokeContext } from './use-core';
-import { Task, TaskFlags, cleanupTask, type DescriptorBase, type Tracker } from './use-task';
+import { Task, TaskFlags, type DescriptorBase, type Tracker } from './use-task';
 
 import type { Container, HostElement, ValueOrPromise } from '../../server/qwik-types';
 import { clearAllEffects } from '../reactive-primitives/cleanup';
@@ -24,6 +24,7 @@ import { delay, isPromise, retryOnPromise, safeCall } from '../shared/utils/prom
 import { isObject } from '../shared/utils/types';
 import { useSequentialScope } from './use-sequential-scope';
 import { cleanupFn, trackFn } from './utils/tracker';
+import { cleanupDestroyable } from './utils/destroyable';
 
 const DEBUG: boolean = false;
 
@@ -265,7 +266,7 @@ export const runResource = <T>(
   host: HostElement
 ): ValueOrPromise<void> => {
   task.$flags$ &= ~TaskFlags.DIRTY;
-  cleanupTask(task);
+  cleanupDestroyable(task);
 
   const iCtx = newInvokeContext(container.$locale$, host, undefined, ResourceEvent);
   iCtx.$container$ = container;
@@ -373,7 +374,7 @@ export const runResource = <T>(
       promise,
       delay(timeout).then(() => {
         if (setState(false, new Error('timeout'))) {
-          cleanupTask(task);
+          cleanupDestroyable(task);
         }
       }),
     ]);

packages/qwik/src/core/use/use-task.ts

@@ -6,7 +6,6 @@ import { assertQrl } from '../shared/qrl/qrl-utils';
 import type { QRL } from '../shared/qrl/qrl.public';
 import { type Container, type HostElement } from '../shared/types';
 import { ChoreType } from '../shared/util-chore-type';
-import { logError } from '../shared/utils/log';
 import { TaskEvent } from '../shared/utils/markers';
 import { isPromise, safeCall } from '../shared/utils/promises';
 import { type NoSerialize } from '../shared/serdes/verify';
@@ -16,6 +15,7 @@ import { useLexicalScope } from './use-lexical-scope.public';
 import type { ResourceReturnInternal } from './use-resource';
 import { useSequentialScope } from './use-sequential-scope';
 import { cleanupFn, trackFn } from './utils/tracker';
+import { cleanupDestroyable } from './utils/destroyable';
 
 export const enum TaskFlags {
   VISIBLE_TASK = 1 << 0,
@@ -168,7 +168,7 @@ export const runTask = (
   host: HostElement
 ): ValueOrPromise<void> => {
   task.$flags$ &= ~TaskFlags.DIRTY;
-  cleanupTask(task);
+  cleanupDestroyable(task);
   const iCtx = newInvokeContext(container.$locale$, host, undefined, TaskEvent);
   iCtx.$container$ = container;
   const taskFn = task.$qrl$.getFn(iCtx, () => clearAllEffects(container, task)) as TaskFn;
@@ -191,18 +191,6 @@ export const runTask = (
   );
 };
 
-export const cleanupTask = (task: Task) => {
-  const destroy = task.$destroy$;
-  if (destroy) {
-    task.$destroy$ = null;
-    try {
-      destroy();
-    } catch (err) {
-      logError(err);
-    }
-  }
-};
-
 export class Task<T = unknown, B = T>
   extends BackRef
   implements DescriptorBase<unknown, Signal<B> | ResourceReturnInternal<B>>

packages/qwik/src/core/use/utils/destroyable.ts

@@ -0,0 +1,22 @@
+import { AsyncComputedSignalImpl } from '../../reactive-primitives/impl/async-computed-signal-impl';
+import type { NoSerialize } from '../../shared/serdes/verify';
+import { logError } from '../../shared/utils/log';
+import { isTask } from '../use-task';
+
+export type Destroyable = { $destroy$: NoSerialize<() => void> | null };
+
+export const cleanupDestroyable = (destroyable: Destroyable) => {
+  const destroy = destroyable.$destroy$;
+  if (destroy) {
+    destroyable.$destroy$ = null;
+    try {
+      destroy();
+    } catch (err) {
+      logError(err);
+    }
+  }
+};
+
+export const isDestroyable = (obj: unknown): obj is Destroyable => {
+  return isTask(obj) || obj instanceof AsyncComputedSignalImpl;
+};

packages/qwik/src/core/use/utils/tracker.ts

@@ -9,13 +9,12 @@ import { getSubscriber } from '../../reactive-primitives/subscriber';
 import { EffectProperty, STORE_ALL_PROPS, type Consumer } from '../../reactive-primitives/types';
 import { isSignal } from '../../reactive-primitives/utils';
 import { qError, QError } from '../../shared/error/error';
+import { noSerialize } from '../../shared/serdes/verify';
 import type { Container } from '../../shared/types';
-import { noSerialize, type NoSerialize } from '../../shared/serdes/verify';
 import { isFunction, isObject } from '../../shared/utils/types';
 import { invoke, newInvokeContext } from '../use-core';
-import type { Tracker } from '../use-task';
-
-export type Destroyable = { $destroy$: NoSerialize<() => void> | null };
+import { type Tracker } from '../use-task';
+import type { Destroyable } from './destroyable';
 
 export const trackFn =
   (target: Consumer, container: Container | null): Tracker =>
@@ -57,13 +56,13 @@ export const cleanupFn = <T extends Destroyable>(
         cleanupFns = [];
         target.$destroy$ = noSerialize(() => {
           target.$destroy$ = null;
-          cleanupFns!.forEach((fn) => {
+          for (const fn of cleanupFns!) {
             try {
               fn();
             } catch (err) {
               handleError(err);
             }
-          });
+          }
         });
       }
       cleanupFns.push(fn);