diff --git a/.changeset/forty-garlics-train.md b/.changeset/forty-garlics-train.md new file mode 100644 index 00000000000..b45123a890d --- /dev/null +++ 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 index 289deb92110..12979d199b5 100644 --- 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 index 0867061eba5..927b65867e8 100644 --- 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 { } 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, [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 index 7682e88d237..9aaccefcf0d 100644 --- 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).$effects$; - const hostElement = (subscriber as WrappedSignal).$hostElement$; - - if (hostElement && hostElement === value) { - (subscriber as WrappedSignal).$hostElement$ = null; + if (subscriptionRemoved) { + effectArray.splice(indexToRemove, 1); } +} + +function clearSignalEffects( + subscriber: Signal, + value: Subscriber | VNode, + seenSet: Set +): 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).$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): 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): 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 index 38804e1dfe9..9cfdf15d09d 100644 --- 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( + 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 index 160f9dfd2f2..1ae69272367 100644 --- 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 { /** 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>( // 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(), ' ')); } diff --git a/packages/qwik/src/core/tests/use-store.spec.tsx b/packages/qwik/src/core/tests/use-store.spec.tsx index 769ac7aa581..c452ac81d1d 100644 --- a/packages/qwik/src/core/tests/use-store.spec.tsx +++ b/packages/qwik/src/core/tests/use-store.spec.tsx @@ -14,6 +14,7 @@ import { import { domRender, ssrRenderToDom, trigger } from '@qwik.dev/core/testing'; import { describe, expect, it, vi } from 'vitest'; import { advanceToNextTimerAndFlush } from '../../testing/element-fixture'; +import { getStoreHandler } from '../signal/store'; const debug = false; //true; Error.stackTraceLimit = 100; @@ -876,6 +877,84 @@ describe.each([ ); }); + it('should cleanup store effects on vNode/component remove', async () => { + (globalThis as any).store = undefined; + + const Child = component$<{ store: { message?: string } }>((props) => { + return
{props.store.message && {props.store.message}}
; + }); + + const Parent = component$(() => { + const store = useStore<{ message?: string }>({ + message: undefined, + }); + + useVisibleTask$(() => { + (globalThis as any).store = store; + }); + + return ( +
+ + +
+ ); + }); + + const { vNode, document } = await render(, { debug }); + + if (render === ssrRenderToDom) { + await trigger(document.body, 'div', 'qvisible'); + } + expect(vNode).toMatchVDOM( + +
+ + +
+
+
+
+ ); + + const storeHandler = getStoreHandler((globalThis as any).store); + expect(storeHandler?.$effects$?.message).toHaveLength(2); + + await trigger(document.body, 'button', 'click'); + expect(storeHandler?.$effects$?.message).toHaveLength(3); + await trigger(document.body, 'button', 'click'); + expect(storeHandler?.$effects$?.message).toHaveLength(2); + await trigger(document.body, 'button', 'click'); + expect(storeHandler?.$effects$?.message).toHaveLength(3); + await trigger(document.body, 'button', 'click'); + expect(storeHandler?.$effects$?.message).toHaveLength(2); + await trigger(document.body, 'button', 'click'); + expect(storeHandler?.$effects$?.message).toHaveLength(3); + + expect(vNode).toMatchVDOM( + +
+ + +
+ + Hello + +
+
+
+
+ ); + }); + describe('regression', () => { it('#5597 - should update value', async () => { (globalThis as any).clicks = 0; diff --git a/packages/qwik/src/core/use/use-resource.ts b/packages/qwik/src/core/use/use-resource.ts index 0790456dfd0..4ff7cc0f455 100644 --- a/packages/qwik/src/core/use/use-resource.ts +++ b/packages/qwik/src/core/use/use-resource.ts @@ -271,7 +271,7 @@ export const runResource = ( const iCtx = newInvokeContext(container.$locale$, host, undefined, ResourceEvent); iCtx.$container$ = container; - const taskFn = task.$qrl$.getFn(iCtx, () => clearSubscriberEffectDependencies(task)); + const taskFn = task.$qrl$.getFn(iCtx, () => clearSubscriberEffectDependencies(container, task)); const resource = task.$state$; assertDefined( diff --git a/packages/qwik/src/core/use/use-task.ts b/packages/qwik/src/core/use/use-task.ts index 19ce8cee00a..b8a68a8e602 100644 --- a/packages/qwik/src/core/use/use-task.ts +++ b/packages/qwik/src/core/use/use-task.ts @@ -190,7 +190,9 @@ export const runTask = ( cleanupTask(task); const iCtx = newInvokeContext(container.$locale$, host, undefined, TaskEvent); iCtx.$container$ = container; - const taskFn = task.$qrl$.getFn(iCtx, () => clearSubscriberEffectDependencies(task)) as TaskFn; + const taskFn = task.$qrl$.getFn(iCtx, () => + clearSubscriberEffectDependencies(container, task) + ) as TaskFn; const track: Tracker = (obj: (() => unknown) | object | Signal, prop?: string) => { const ctx = newInvokeContext();