From 33857ae1afcda406ace0a1047671b62c13cbd819 Mon Sep 17 00:00:00 2001 From: Varixo Date: Thu, 15 Aug 2024 21:34:31 +0200 Subject: [PATCH] implement signals cleanup --- packages/qwik/src/core/debug.ts | 14 ++++- packages/qwik/src/core/use/use-task.ts | 18 ++++-- packages/qwik/src/core/util/markers.ts | 1 + .../qwik/src/core/v2/client/vnode-diff.ts | 5 +- .../core/v2/shared/shared-serialization.ts | 19 ++++++- .../v2/shared/shared-serialization.unit.ts | 3 +- packages/qwik/src/core/v2/signal/v2-signal.ts | 55 ++++++++++++++++-- .../qwik/src/core/v2/signal/v2-subscriber.ts | 57 +++++++++++++++++++ 8 files changed, 154 insertions(+), 18 deletions(-) create mode 100644 packages/qwik/src/core/v2/signal/v2-subscriber.ts diff --git a/packages/qwik/src/core/debug.ts b/packages/qwik/src/core/debug.ts index 949060467b8..63aa140dfc0 100644 --- a/packages/qwik/src/core/debug.ts +++ b/packages/qwik/src/core/debug.ts @@ -2,7 +2,7 @@ import { isQrl } from '../server/prefetch-strategy'; import { isJSXNode } from './render/jsx/jsx-runtime'; import { isTask } from './use/use-task'; import { vnode_isVNode, vnode_toString } from './v2/client/vnode'; -import { isSignal2 } from './v2/signal/v2-signal'; +import { ComputedSignal2, DerivedSignal2, isSignal2 } from './v2/signal/v2-signal'; import { isStore2 } from './v2/signal/v2-store'; const stringifyPath: any[] = []; @@ -34,8 +34,16 @@ export function qwikDebugToString(value: any): any { } else { return value.map(qwikDebugToString); } - } else if (isStore2(value) || isSignal2(value)) { - return value.toString(); + } else if (isSignal2(value)) { + if (value instanceof DerivedSignal2) { + return 'DerivedSignal(' + qwikDebugToString(value.untrackedValue) + ')'; + } else if (value instanceof ComputedSignal2) { + return 'ComputedSignal(' + qwikDebugToString(value.untrackedValue) + ')'; + } else { + return 'Signal(' + qwikDebugToString(value.untrackedValue) + ')'; + } + } else if (isStore2(value)) { + return 'Store(' + qwikDebugToString(value) + ')'; } else if (isJSXNode(value)) { return jsxToString(value); } diff --git a/packages/qwik/src/core/use/use-task.ts b/packages/qwik/src/core/use/use-task.ts index 375d507e3e6..26d0961b485 100644 --- a/packages/qwik/src/core/use/use-task.ts +++ b/packages/qwik/src/core/use/use-task.ts @@ -36,6 +36,7 @@ import { } from '../v2/signal/v2-signal'; import { type ReadonlySignal2, type Signal2 } from '../v2/signal/v2-signal.public'; import { unwrapStore2 } from '../v2/signal/v2-store'; +import { clearSubscriberDependencies, Subscriber } from '../v2/signal/v2-subscriber'; import { invoke, newInvokeContext, untrack, waitAndRun } from './use-core'; import { useOn, useOnDocument } from './use-on'; import { useSequentialScope } from './use-sequential-scope'; @@ -194,7 +195,7 @@ export interface ResourceReturnInternal extends Record { loading: boolean; } /** @public */ -export interface DescriptorBase { +export interface DescriptorBase extends Subscriber { $flags$: number; $index$: number; $el$: QwikElement; @@ -350,7 +351,7 @@ export const runTask2 = (task: Task, container: Container2, host: HostElement) = task.$flags$ &= ~TaskFlags.DIRTY; const iCtx = newInvokeContext(container.$locale$, host as fixMeAny, undefined, TaskEvent); iCtx.$container2$ = container; - const taskFn = task.$qrl$.getFn(iCtx) as TaskFn; + const taskFn = task.$qrl$.getFn(iCtx, () => clearSubscriberDependencies(task)) as TaskFn; const track: Tracker = (obj: (() => unknown) | object | Signal, prop?: string) => { const ctx = newInvokeContext(); @@ -363,7 +364,7 @@ export const runTask2 = (task: Task, container: Container2, host: HostElement) = if (prop) { return (obj as Record)[prop]; } else if (isSignal2(obj)) { - return obj.untrackedValue; + return obj.value; } else { return obj; } @@ -568,7 +569,7 @@ export const runResource = ( const iCtx = newInvokeContext(container.$locale$, host as fixMeAny, undefined, ResourceEvent); iCtx.$container2$ = container; - const taskFn = task.$qrl$.getFn(iCtx); + const taskFn = task.$qrl$.getFn(iCtx, () => clearSubscriberDependencies(task)); const resource = task.$state$; assertDefined( @@ -859,7 +860,10 @@ export const parseTask = (data: string) => { return new Task(strToInt(flags), strToInt(index), el as any, qrl as any, resource as any, null); }; -export class Task implements DescriptorBase> { +export class Task + extends Subscriber + implements DescriptorBase> +{ constructor( public $flags$: number, public $index$: number, @@ -867,7 +871,9 @@ export class Task implements DescriptorBase, public $state$: Signal | undefined, public $destroy$: NoSerialize<() => void> | null - ) {} + ) { + super(); + } } export const isTask = (value: any): value is Task => { diff --git a/packages/qwik/src/core/util/markers.ts b/packages/qwik/src/core/util/markers.ts index 082c251809d..4a9eae3d6c9 100644 --- a/packages/qwik/src/core/util/markers.ts +++ b/packages/qwik/src/core/util/markers.ts @@ -25,6 +25,7 @@ export const QStyleSSelector = 'style[q\\:sstyle]'; export const QStylesAllSelector = QStyleSelector + ',' + QStyleSSelector; export const QScopedStyle = 'q:sstyle'; export const QCtxAttr = 'q:ctx'; +export const QSubscribers = 'q:subs'; export const QRenderAttr = 'q:render'; export const QRuntimeAttr = 'q:runtime'; diff --git a/packages/qwik/src/core/v2/client/vnode-diff.ts b/packages/qwik/src/core/v2/client/vnode-diff.ts index 1cec5f6c2d1..72563f5d663 100644 --- a/packages/qwik/src/core/v2/client/vnode-diff.ts +++ b/packages/qwik/src/core/v2/client/vnode-diff.ts @@ -97,6 +97,7 @@ import type { Signal2 } from '../signal/v2-signal.public'; import { executeComponent2 } from '../shared/component-execution'; import { isParentSlotProp, isSlotProp } from '../../util/prop'; import { escapeHTML } from '../shared/character-escaping'; +import { clearSubscriberDependencies, clearVNodeDependencies } from '../signal/v2-subscriber'; export type ComponentQueue = Array; @@ -1206,12 +1207,14 @@ export function cleanup(container: ClientContainer, vNode: VNode) { // Only elements and virtual nodes need to be traversed for children if (type & VNodeFlags.Virtual) { // Only virtual nodes have subscriptions - const seq = container.getHostProp>(vCursor as fixMeAny, ELEMENT_SEQ); + clearVNodeDependencies(vCursor); + const seq = container.getHostProp>(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; + clearSubscriberDependencies(task); if (obj.$flags$ & TaskFlags.VISIBLE_TASK) { container.$scheduler$(ChoreType.CLEANUP_VISIBLE, obj); } else { diff --git a/packages/qwik/src/core/v2/shared/shared-serialization.ts b/packages/qwik/src/core/v2/shared/shared-serialization.ts index 630fbe733f7..f1bd478f3c2 100644 --- a/packages/qwik/src/core/v2/shared/shared-serialization.ts +++ b/packages/qwik/src/core/v2/shared/shared-serialization.ts @@ -48,6 +48,7 @@ import { unwrapStore2, type StoreHandler, } from '../signal/v2-store'; +import type { Subscriber } from '../signal/v2-subscriber'; import type { SymbolToChunkResolver } from '../ssr/ssr-types'; import type { DeserializeContainer, fixMeAny } from './types'; @@ -298,6 +299,7 @@ const inflate = (container: DeserializeContainer, target: any, needsInflationDat task.$flags$ = restInt(); task.$index$ = restInt(); task.$el$ = container.$getObjectById$(restInt()) as Element; + task.$dependencies$ = container.$getObjectById$(restInt()) as Subscriber[] | null; task.$qrl$ = inflateQRL(container, parseQRL(restString())); const taskState = restString(); task.$state$ = taskState @@ -736,9 +738,14 @@ export const createSerializationContext = ( discoveredValues.push(tuples); } else if (obj instanceof Signal2) { discoveredValues.push(obj.$untrackedValue$); + if (obj.$effects$) { + for (const effect of obj.$effects$) { + discoveredValues.push(effect[EffectSubscriptionsProp.EFFECT]); + } + } // TODO(mhevery): should scan the QRLs??? } else if (obj instanceof Task) { - discoveredValues.push(obj.$el$, obj.$qrl$, obj.$state$); + discoveredValues.push(obj.$el$, obj.$qrl$, obj.$state$, obj.$dependencies$); } else if (NodeConstructor && obj instanceof NodeConstructor) { // ignore the nodes // debugger; @@ -900,6 +907,8 @@ function serialize(serializationContext: SerializationContext): void { SerializationConstant.DerivedSignal_CHAR + serializeDerivedFn(serializationContext, value, $addRoot$) + ';' + + $addRoot$(value.$dependencies$) + + ';' + $addRoot$(value.$untrackedValue$) + serializeEffectSubs($addRoot$, value.$effects$) ); @@ -908,12 +917,16 @@ function serialize(serializationContext: SerializationContext): void { SerializationConstant.ComputedSignal_CHAR + qrlToString(serializationContext, value.$computeQrl$) + ';' + + $addRoot$(value.$dependencies$) + + ';' + $addRoot$(value.$untrackedValue$) + serializeEffectSubs($addRoot$, value.$effects$) ); } else { writeString( SerializationConstant.Signal_CHAR + + $addRoot$(value.$dependencies$) + + ';' + $addRoot$(value.$untrackedValue$) + serializeEffectSubs($addRoot$, value.$effects$) ); @@ -977,6 +990,8 @@ function serialize(serializationContext: SerializationContext): void { ' ' + $addRoot$(value.$el$) + ' ' + + $addRoot$(value.$dependencies$) + + ' ' + qrlToString(serializationContext, value.$qrl$) + (value.$state$ == null ? '' : ' ' + $addRoot$(value.$state$)) ); @@ -1109,6 +1124,8 @@ function deserializeSignal2( const computedSignal = signal as ComputedSignal2; computedSignal.$computeQrl$ = inflateQRL(container, parseQRL(parts[idx++])) as fixMeAny; } + const dependencies = container.$getObjectById$(parts[idx++]) as Subscriber[] | null; + signal.$dependencies$ = dependencies; let signalValue = container.$getObjectById$(parts[idx++]); if (vnode_isVNode(signalValue)) { signalValue = vnode_getNode(signalValue); diff --git a/packages/qwik/src/core/v2/shared/shared-serialization.unit.ts b/packages/qwik/src/core/v2/shared/shared-serialization.unit.ts index 40509fc5b4c..546a83e3588 100644 --- a/packages/qwik/src/core/v2/shared/shared-serialization.unit.ts +++ b/packages/qwik/src/core/v2/shared/shared-serialization.unit.ts @@ -60,8 +60,9 @@ describe('shared-serialization', () => { new Task(0, 0, shared1 as any, qrl, shared2 as any, null) ); expect(objs).toEqual([ - SerializationConstant.Task_CHAR + '0 0 1 qwik-runtime-mock-chunk#s_zero 2', + SerializationConstant.Task_CHAR + '0 0 1 2 qwik-runtime-mock-chunk#s_zero 3', shared1, + null, shared2, ]); }); diff --git a/packages/qwik/src/core/v2/signal/v2-signal.ts b/packages/qwik/src/core/v2/signal/v2-signal.ts index 9ee6cfc1b27..cb395cbaa05 100644 --- a/packages/qwik/src/core/v2/signal/v2-signal.ts +++ b/packages/qwik/src/core/v2/signal/v2-signal.ts @@ -18,15 +18,17 @@ import { type QRLInternal } from '../../qrl/qrl-class'; import type { QRL } from '../../qrl/qrl.public'; import { trackSignal2, tryGetInvokeContext } from '../../use/use-core'; import { Task, TaskFlags, isTask } from '../../use/use-task'; -import { ELEMENT_PROPS, OnRenderProp } from '../../util/markers'; +import { ELEMENT_PROPS, OnRenderProp, QSubscribers } from '../../util/markers'; import { isPromise } from '../../util/promises'; import { qDev } from '../../util/qdev'; import type { VNode } from '../client/types'; +import { vnode_getProp, vnode_isVirtualVNode, vnode_isVNode, vnode_setProp } from '../client/vnode'; import { ChoreType } from '../shared/scheduler'; import type { Container2, HostElement, fixMeAny } from '../shared/types'; import type { ISsrNode } from '../ssr/ssr-types'; import type { Signal2 as ISignal2 } from './v2-signal.public'; import type { Store2 } from './v2-store'; +import { isSubscriber, Subscriber } from './v2-subscriber'; const DEBUG = false; @@ -135,7 +137,7 @@ export const enum EffectProperty { VNODE = '.', } -export class Signal2 implements ISignal2 { +export class Signal2 extends Subscriber implements ISignal2 { $untrackedValue$: T; /** Store a list of effects which are dependent on this signal. */ @@ -144,6 +146,7 @@ export class Signal2 implements ISignal2 { $container$: Container2 | null = null; constructor(container: Container2 | null, value: T) { + super(); this.$container$ = container; this.$untrackedValue$ = value; DEBUG && log('new', this); @@ -183,6 +186,8 @@ export class Signal2 implements ISignal2 { // to unsubscribe from. So we need to store the reference from the effect back // to this signal. ensureContains(effectSubscriber, this); + // We need to add the subscriber to the effect so that we can clean it up later + ensureEffectContainsSubscriber(effectSubscriber[EffectSubscriptionsProp.EFFECT], this); DEBUG && log('read->sub', pad('\n' + this.toString(), ' ')); } } @@ -224,14 +229,51 @@ export const ensureContains = (array: any[], value: any) => { } }; -export const ensureContainsEffect = (array: EffectSubscriptions[], effect: EffectSubscriptions) => { +export const ensureContainsEffect = ( + array: EffectSubscriptions[], + effectSubscriptions: EffectSubscriptions +) => { for (let i = 0; i < array.length; i++) { const existingEffect = array[i]; - if (existingEffect[0] === effect[0] && existingEffect[1] === effect[1]) { + if ( + existingEffect[0] === effectSubscriptions[0] && + existingEffect[1] === effectSubscriptions[1] + ) { return; } } - array.push(effect); + array.push(effectSubscriptions); +}; + +export const ensureEffectContainsSubscriber = (effect: Effect, subscriber: Subscriber) => { + if (isSubscriber(effect)) { + effect.$dependencies$ ||= []; + + if (subscriberExistInSubscribers(effect.$dependencies$, subscriber)) { + return; + } + + effect.$dependencies$.push(subscriber); + } else if (vnode_isVNode(effect) && vnode_isVirtualVNode(effect)) { + let subscribers = vnode_getProp(effect, QSubscribers, null); + subscribers ||= []; + + if (subscriberExistInSubscribers(subscribers, subscriber)) { + return; + } + + subscribers.push(subscriber); + vnode_setProp(effect, QSubscribers, subscribers); + } +}; + +const subscriberExistInSubscribers = (subscribers: Subscriber[], subscriber: Subscriber) => { + for (let i = 0; i < subscribers.length; i++) { + if (subscribers[i] === subscriber) { + return true; + } + } + return false; }; export const triggerEffects = ( @@ -388,7 +430,8 @@ export class ComputedSignal2 extends Signal2 { throw new TypeError('ComputedSignal is read-only'); } } - +// TO DISCUSS (with Misko): Shouldn't it be called a "WrappedSignal" ? +// Plus - shouldn't this type of signal have the $dependencies$ array instead of EVERY type of signal? export class DerivedSignal2 extends Signal2 { $args$: any[]; $func$: (...args: any[]) => T; diff --git a/packages/qwik/src/core/v2/signal/v2-subscriber.ts b/packages/qwik/src/core/v2/signal/v2-subscriber.ts new file mode 100644 index 00000000000..a44ed9c7c20 --- /dev/null +++ b/packages/qwik/src/core/v2/signal/v2-subscriber.ts @@ -0,0 +1,57 @@ +import { QSubscribers } from '../../util/markers'; +import type { VNode } from '../client/types'; +import { vnode_getProp } from '../client/vnode'; +import { EffectSubscriptionsProp, isSignal2, type Signal2 } from './v2-signal'; + +export abstract class Subscriber { + $dependencies$: Subscriber[] | null = null; +} + +export function isSubscriber(value: unknown): value is Subscriber { + return value instanceof Subscriber; +} + +export function clearVNodeDependencies(value: VNode): void { + const effects = vnode_getProp(value, QSubscribers, null); + if (!effects) { + return; + } + for (let i = effects.length - 1; i >= 0; i--) { + const subscriber = effects[i]; + const subscriptionRemoved = clearSubscriptions(subscriber, value); + if (subscriptionRemoved) { + effects.splice(i, 1); + } + } +} + +export function clearSubscriberDependencies(value: Subscriber): void { + if (value.$dependencies$) { + for (let i = value.$dependencies$.length - 1; i >= 0; i--) { + const subscriber = value.$dependencies$[i]; + const subscriptionRemoved = clearSubscriptions(subscriber, value); + if (subscriptionRemoved) { + value.$dependencies$.splice(i, 1); + } + } + } +} + +function clearSubscriptions(subscriber: Subscriber, value: Subscriber | VNode): boolean { + if (!isSignal2(subscriber)) { + return false; + } + const effectSubscriptions = (subscriber as Signal2).$effects$; + if (!effectSubscriptions) { + return false; + } + let subscriptionRemoved = false; + for (let i = effectSubscriptions.length - 1; i >= 0; i--) { + const effect = effectSubscriptions[i]; + if (effect[EffectSubscriptionsProp.EFFECT] === value) { + effectSubscriptions.splice(i, 1); + subscriptionRemoved = true; + } + } + return subscriptionRemoved; +}