Skip to content

Commit

Permalink
implement signals cleanup
Browse files Browse the repository at this point in the history
  • Loading branch information
Varixo committed Aug 16, 2024
1 parent a4dc963 commit 33857ae
Show file tree
Hide file tree
Showing 8 changed files with 154 additions and 18 deletions.
14 changes: 11 additions & 3 deletions packages/qwik/src/core/debug.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = [];
Expand Down Expand Up @@ -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);
}
Expand Down
18 changes: 12 additions & 6 deletions packages/qwik/src/core/use/use-task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -194,7 +195,7 @@ export interface ResourceReturnInternal<T> extends Record<string, unknown> {
loading: boolean;
}
/** @public */
export interface DescriptorBase<T = unknown, B = unknown> {
export interface DescriptorBase<T = unknown, B = unknown> extends Subscriber {
$flags$: number;
$index$: number;
$el$: QwikElement;
Expand Down Expand Up @@ -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();
Expand All @@ -363,7 +364,7 @@ export const runTask2 = (task: Task, container: Container2, host: HostElement) =
if (prop) {
return (obj as Record<string, unknown>)[prop];
} else if (isSignal2(obj)) {
return obj.untrackedValue;
return obj.value;
} else {
return obj;
}
Expand Down Expand Up @@ -568,7 +569,7 @@ export const runResource = <T>(
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(
Expand Down Expand Up @@ -859,15 +860,20 @@ 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<T = unknown, B = T> implements DescriptorBase<unknown, Signal<B>> {
export class Task<T = unknown, B = T>
extends Subscriber
implements DescriptorBase<unknown, Signal<B>>
{
constructor(
public $flags$: number,
public $index$: number,
public $el$: QwikElement,
public $qrl$: QRLInternal<T>,
public $state$: Signal<B> | undefined,
public $destroy$: NoSerialize<() => void> | null
) {}
) {
super();
}
}

export const isTask = (value: any): value is Task => {
Expand Down
1 change: 1 addition & 0 deletions packages/qwik/src/core/util/markers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
5 changes: 4 additions & 1 deletion packages/qwik/src/core/v2/client/vnode-diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<VNode>;

Expand Down Expand Up @@ -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<Array<any>>(vCursor as fixMeAny, ELEMENT_SEQ);
clearVNodeDependencies(vCursor);
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;
clearSubscriberDependencies(task);
if (obj.$flags$ & TaskFlags.VISIBLE_TASK) {
container.$scheduler$(ChoreType.CLEANUP_VISIBLE, obj);
} else {
Expand Down
19 changes: 18 additions & 1 deletion packages/qwik/src/core/v2/shared/shared-serialization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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$)
);
Expand All @@ -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$)
);
Expand Down Expand Up @@ -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$))
);
Expand Down Expand Up @@ -1109,6 +1124,8 @@ function deserializeSignal2(
const computedSignal = signal as ComputedSignal2<any>;
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
]);
});
Expand Down
55 changes: 49 additions & 6 deletions packages/qwik/src/core/v2/signal/v2-signal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -135,7 +137,7 @@ export const enum EffectProperty {
VNODE = '.',
}

export class Signal2<T = any> implements ISignal2<T> {
export class Signal2<T = any> extends Subscriber implements ISignal2<T> {
$untrackedValue$: T;

/** Store a list of effects which are dependent on this signal. */
Expand All @@ -144,6 +146,7 @@ export class Signal2<T = any> implements ISignal2<T> {
$container$: Container2 | null = null;

constructor(container: Container2 | null, value: T) {
super();
this.$container$ = container;
this.$untrackedValue$ = value;
DEBUG && log('new', this);
Expand Down Expand Up @@ -183,6 +186,8 @@ export class Signal2<T = any> implements ISignal2<T> {
// 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(), ' '));
}
}
Expand Down Expand Up @@ -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<Subscriber[]>(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 = (
Expand Down Expand Up @@ -388,7 +430,8 @@ export class ComputedSignal2<T> extends Signal2<T> {
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<T> extends Signal2<T> {
$args$: any[];
$func$: (...args: any[]) => T;
Expand Down
57 changes: 57 additions & 0 deletions packages/qwik/src/core/v2/signal/v2-subscriber.ts
Original file line number Diff line number Diff line change
@@ -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<Subscriber[]>(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<unknown>).$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;
}

0 comments on commit 33857ae

Please sign in to comment.