fix: store effects cleanup

QwikDev · Jan 5, 2025 · 5bcaf63 · 5bcaf63
1 parent a4862af
commit 5bcaf63
Show file tree

Hide file tree

Showing 9 changed files with 236 additions and 57 deletions.
diff --git a/.changeset/forty-garlics-train.md b/.changeset/forty-garlics-train.md
@@ -0,0 +1,5 @@
+---
+'@qwik.dev/core': patch
+---
+
+fix: store effects cleanup
diff --git a/packages/qwik/src/core/client/vnode-diff.ts b/packages/qwik/src/core/client/vnode-diff.ts
@@ -1288,7 +1288,7 @@ export function cleanup(container: ClientContainer, vNode: VNode) {
             const obj = seq[i];
             if (isTask(obj)) {
               const task = obj;
-              clearSubscriberEffectDependencies(task);
+              clearSubscriberEffectDependencies(container, task);
               if (task.$flags$ & TaskFlags.VISIBLE_TASK) {
                 container.$scheduler$(ChoreType.CLEANUP_VISIBLE, task);
               } else {

diff --git a/packages/qwik/src/core/shared/shared-serialization.ts b/packages/qwik/src/core/shared/shared-serialization.ts
@@ -12,7 +12,7 @@ import { ComputedSignal, EffectPropData, Signal, WrappedSignal } from '../signal
 import type { Subscriber } from '../signal/signal-subscriber';
 import {
   STORE_ARRAY_PROP,
-  createStore,
+  getOrCreateStore,
   getStoreHandler,
   getStoreTarget,
   isStore,
@@ -121,14 +121,15 @@ class DeserializationHandler implements ProxyHandler<object> {
     }
 
     const container = this.$container$;
-    const propValue = allocate(container, typeId, value);
-    Reflect.set(target, property, propValue);
-    this.$data$[idx] = undefined;
-    this.$data$[idx + 1] = propValue;
+    let propValue = allocate(container, typeId, value);
     /** We stored the reference, so now we can inflate, allowing cycles. */
     if (typeId >= TypeIds.Error) {
-      inflate(container, propValue, typeId, value);
+      propValue = inflate(container, propValue, typeId, value);
     }
+
+    Reflect.set(target, property, propValue);
+    this.$data$[idx] = undefined;
+    this.$data$[idx + 1] = propValue;
     return propValue;
   }
 
@@ -172,10 +173,15 @@ export const _eagerDeserializeArray = (
 
 const resolvers = new WeakMap<Promise<any>, [Function, Function]>();
 
-const inflate = (container: DeserializeContainer, target: any, typeId: TypeIds, data: unknown) => {
+const inflate = (
+  container: DeserializeContainer,
+  target: any,
+  typeId: TypeIds,
+  data: unknown
+): any => {
   if (typeId === undefined) {
     // Already processed
-    return;
+    return target;
   }
   // restore the complex data, except for plain objects
   if (typeId !== TypeIds.Object && Array.isArray(data)) {
@@ -249,17 +255,14 @@ const inflate = (container: DeserializeContainer, target: any, typeId: TypeIds,
     case TypeIds.Store:
     case TypeIds.StoreArray: {
       const [value, flags, effects, storeEffect] = data as unknown[];
-      const handler = getStoreHandler(target)!;
-      handler.$flags$ = flags as number;
-      // First assign so it sets up the deep stores
-      Object.assign(getStoreTarget(target), value as object);
-      // Afterwards restore the effects so they don't get triggered
+      const store = getOrCreateStore(value as object, flags as number, container as DomContainer);
+      const storeHandler = getStoreHandler(store)!;
       if (storeEffect) {
         (effects as any)[STORE_ARRAY_PROP] = storeEffect;
       }
-      handler.$effects$ = effects as any;
+      storeHandler.$effects$ = effects as any;
+      target = store;
 
-      container.$storeProxyMap$.set(value, target);
       break;
     }
     case TypeIds.Signal: {
@@ -387,6 +390,7 @@ const inflate = (container: DeserializeContainer, target: any, typeId: TypeIds,
     default:
       throw qError(QError.serializeErrorNotImplemented, [typeId]);
   }
+  return target;
 };
 
 export const _constants = [
@@ -475,9 +479,9 @@ const allocate = (container: DeserializeContainer, typeId: number, value: unknow
     case TypeIds.ComputedSignal:
       return new ComputedSignal(container as any, null!);
     case TypeIds.Store:
-      return createStore(container as any, {}, 0);
     case TypeIds.StoreArray:
-      return createStore(container as any, [], 0);
+      // ignore allocate, we need to assign target while creating store
+      return null;
     case TypeIds.URLSearchParams:
       return new URLSearchParams(value as string);
     case TypeIds.FormData:
@@ -1420,15 +1424,15 @@ export function _deserialize(rawStateData: string | null, element?: unknown): un
   return output;
 }
 
-function deserializeData(container: DeserializeContainer, typeId: number, propValue: unknown) {
+function deserializeData(container: DeserializeContainer, typeId: number, value: unknown) {
   if (typeId === undefined) {
-    return propValue;
+    return value;
   }
-  const value = allocate(container, typeId, propValue);
+  let propValue = allocate(container, typeId, value);
   if (typeId >= TypeIds.Error) {
-    inflate(container, value, typeId, propValue);
+    propValue = inflate(container, propValue, typeId, value);
   }
-  return value;
+  return propValue;
 }
 
 function getObjectById(id: number | string, stateData: unknown[]): unknown {

diff --git a/packages/qwik/src/core/signal/signal-subscriber.ts b/packages/qwik/src/core/signal/signal-subscriber.ts
@@ -1,11 +1,17 @@
 import { QSubscribers } from '../shared/utils/markers';
-import type { VNode } from '../client/types';
-import { ensureMaterialized, vnode_getProp, vnode_isElementVNode } from '../client/vnode';
-import { EffectSubscriptionsProp, WrappedSignal, isSignal } from './signal';
+import type { ElementVNode, VNode, VirtualVNode } from '../client/types';
+import {
+  ensureMaterialized,
+  vnode_getProp,
+  vnode_isElementVNode,
+  vnode_setProp,
+} from '../client/vnode';
+import { EffectSubscriptionsProp, WrappedSignal, isSignal, type Signal } from './signal';
 import type { Container } from '../shared/types';
+import { StoreHandler, getStoreHandler, isStore, type TargetType } from './store';
 
 export abstract class Subscriber {
-  $effectDependencies$: Subscriber[] | null = null;
+  $effectDependencies$: (Subscriber | TargetType)[] | null = null;
 }
 
 export function isSubscriber(value: unknown): value is Subscriber {
@@ -22,35 +28,55 @@ export function clearVNodeEffectDependencies(container: Container, value: VNode)
   }
   for (let i = effects.length - 1; i >= 0; i--) {
     const subscriber = effects[i];
-    const subscriptionRemoved = clearEffects(subscriber, value);
-    if (subscriptionRemoved) {
-      effects.splice(i, 1);
-    }
+    clearEffects(subscriber, value, effects, i, container);
+  }
+
+  if (effects.length === 0) {
+    vnode_setProp(value as ElementVNode | VirtualVNode, QSubscribers, null);
   }
 }
 
-export function clearSubscriberEffectDependencies(value: Subscriber): void {
+export function clearSubscriberEffectDependencies(container: Container, value: Subscriber): void {
   if (value.$effectDependencies$) {
     for (let i = value.$effectDependencies$.length - 1; i >= 0; i--) {
       const subscriber = value.$effectDependencies$[i];
-      const subscriptionRemoved = clearEffects(subscriber, value);
-      if (subscriptionRemoved) {
-        value.$effectDependencies$.splice(i, 1);
-      }
+      clearEffects(subscriber, value, value.$effectDependencies$, i, container);
+    }
+
+    if (value.$effectDependencies$.length === 0) {
+      value.$effectDependencies$ = null;
     }
   }
 }
 
-function clearEffects(subscriber: Subscriber, value: Subscriber | VNode): boolean {
-  if (!isSignal(subscriber)) {
-    return false;
+function clearEffects(
+  subscriber: Subscriber | TargetType,
+  value: Subscriber | VNode,
+  effectArray: (Subscriber | TargetType)[],
+  indexToRemove: number,
+  container: Container
+) {
+  let subscriptionRemoved = false;
+  const seenSet = new Set();
+  if (subscriber instanceof WrappedSignal) {
+    subscriptionRemoved = clearSignalEffects(subscriber, value, seenSet);
+  } else if (container.$storeProxyMap$.has(subscriber)) {
+    const store = container.$storeProxyMap$.get(subscriber)!;
+    const handler = getStoreHandler(store)!;
+    subscriptionRemoved = clearStoreEffects(handler, value);
   }
-  const effectSubscriptions = (subscriber as WrappedSignal<unknown>).$effects$;
-  const hostElement = (subscriber as WrappedSignal<unknown>).$hostElement$;
-
-  if (hostElement && hostElement === value) {
-    (subscriber as WrappedSignal<unknown>).$hostElement$ = null;
+  if (subscriptionRemoved) {
+    effectArray.splice(indexToRemove, 1);
   }
+}
+
+function clearSignalEffects(
+  subscriber: Signal,
+  value: Subscriber | VNode,
+  seenSet: Set<unknown>
+): boolean {
+  const effectSubscriptions = subscriber.$effects$;
+
   let subscriptionRemoved = false;
   if (effectSubscriptions) {
     for (let i = effectSubscriptions.length - 1; i >= 0; i--) {
@@ -62,13 +88,67 @@ function clearEffects(subscriber: Subscriber, value: Subscriber | VNode): boolea
     }
   }
 
-  // clear the effects of the arguments
-  const args = (subscriber as WrappedSignal<unknown>).$args$;
-  if (args) {
-    for (let i = args.length - 1; i >= 0; i--) {
-      clearEffects(args[i], subscriber);
+  if (subscriber instanceof WrappedSignal) {
+    const hostElement = subscriber.$hostElement$;
+    if (hostElement && hostElement === value) {
+      subscriber.$hostElement$ = null;
+    }
+
+    // clear the effects of the arguments
+    const args = subscriber.$args$;
+    if (args) {
+      clearArgsEffects(args, subscriber, seenSet);
+    }
+  }
+
+  return subscriptionRemoved;
+}
+
+function clearStoreEffects(storeHandler: StoreHandler, value: Subscriber | VNode): boolean {
+  const effectSubscriptions = storeHandler.$effects$;
+  if (!effectSubscriptions) {
+    return false;
+  }
+  let subscriptionRemoved = false;
+  for (const key in effectSubscriptions) {
+    const effects = effectSubscriptions[key];
+    for (let i = effects.length - 1; i >= 0; i--) {
+      const effect = effects[i];
+      if (effect[EffectSubscriptionsProp.EFFECT] === value) {
+        effects.splice(i, 1);
+        subscriptionRemoved = true;
+      }
     }
   }
 
   return subscriptionRemoved;
 }
+
+function clearArgsEffects(args: any[], subscriber: Subscriber, seenSet: Set<unknown>): void {
+  for (let i = args.length - 1; i >= 0; i--) {
+    const arg = args[i];
+    clearArgEffect(arg, subscriber, seenSet);
+  }
+}
+
+function clearArgEffect(arg: any, subscriber: Subscriber, seenSet: Set<unknown>): void {
+  if (seenSet.has(arg)) {
+    return;
+  }
+  seenSet.add(arg);
+  if (isSignal(arg)) {
+    clearSignalEffects(arg as Signal, subscriber, seenSet);
+  } else if (typeof arg === 'object' && arg !== null) {
+    if (isStore(arg)) {
+      clearStoreEffects(getStoreHandler(arg)!, subscriber);
+    } else {
+      for (const key in arg) {
+        clearArgEffect(arg[key], subscriber, seenSet);
+      }
+    }
+  } else if (Array.isArray(arg)) {
+    clearArgsEffects(arg, subscriber, seenSet);
+  } else {
+    // primitives
+  }
+}
diff --git a/packages/qwik/src/core/signal/signal.ts b/packages/qwik/src/core/signal/signal.ts
@@ -259,7 +259,7 @@ export const ensureContainsEffect = (
 
 export const ensureEffectContainsSubscriber = (
   effect: Effect,
-  subscriber: Subscriber,
+  subscriber: Subscriber | TargetType,
   container: Container | null
 ) => {
   if (isSubscriber(effect)) {
@@ -271,7 +271,7 @@ export const ensureEffectContainsSubscriber = (
 
     effect.$effectDependencies$.push(subscriber);
   } else if (vnode_isVNode(effect) && !vnode_isTextVNode(effect)) {
-    let subscribers = vnode_getProp<Subscriber[]>(
+    let subscribers = vnode_getProp<(Subscriber | TargetType)[]>(
       effect,
       QSubscribers,
       container ? container.$getObjectById$ : null
@@ -285,7 +285,7 @@ export const ensureEffectContainsSubscriber = (
     subscribers.push(subscriber);
     vnode_setProp(effect, QSubscribers, subscribers);
   } else if (isSSRNode(effect)) {
-    let subscribers = effect.getProp(QSubscribers) as Subscriber[];
+    let subscribers = effect.getProp(QSubscribers) as (Subscriber | TargetType)[];
     subscribers ||= [];
 
     if (subscriberExistInSubscribers(subscribers, subscriber)) {
@@ -301,7 +301,10 @@ const isSSRNode = (effect: Effect): effect is ISsrNode => {
   return 'setProp' in effect && 'getProp' in effect && 'removeProp' in effect && 'id' in effect;
 };
 
-const subscriberExistInSubscribers = (subscribers: Subscriber[], subscriber: Subscriber) => {
+const subscriberExistInSubscribers = (
+  subscribers: (Subscriber | TargetType)[],
+  subscriber: Subscriber | TargetType
+) => {
   for (let i = 0; i < subscribers.length; i++) {
     if (subscribers[i] === subscriber) {
       return true;

diff --git a/packages/qwik/src/core/signal/store.ts b/packages/qwik/src/core/signal/store.ts
@@ -2,11 +2,12 @@ import { pad, qwikDebugToString } from '../debug';
 import { assertTrue } from '../shared/error/assert';
 import { tryGetInvokeContext } from '../use/use-core';
 import { isSerializableObject } from '../shared/utils/types';
-import { unwrapDeserializerProxy } from '../shared/shared-serialization';
 import type { Container } from '../shared/types';
 import {
+  EffectSubscriptionsProp,
   ensureContains,
   ensureContainsEffect,
+  ensureEffectContainsSubscriber,
   triggerEffects,
   type EffectSubscriptions,
 } from './signal';
@@ -137,7 +138,6 @@ export class StoreHandler implements ProxyHandler<TargetType> {
 
   /** In the case of oldValue and value are the same, the effects are not triggered. */
   set(target: TargetType, prop: string | symbol, value: any): boolean {
-    target = unwrapDeserializerProxy(target) as TargetType;
     if (typeof prop === 'symbol') {
       target[prop] = value;
       return true;
@@ -227,6 +227,12 @@ function addEffect<T extends Record<string | symbol, any>>(
   // to unsubscribe from. So we need to store the reference from the effect back
   // to this signal.
   ensureContains(effectSubscriber, target);
+  // We need to add the subscriber to the effect so that we can clean it up later
+  ensureEffectContainsSubscriber(
+    effectSubscriber[EffectSubscriptionsProp.EFFECT],
+    target,
+    store.$container$
+  );
 
   DEBUG && log('sub', pad('\n' + store.$effects$.toString(), '  '));
 }