From 9aab08e652a4a22d1ed14ebbd3adc02cad76edb0 Mon Sep 17 00:00:00 2001 From: Varixo Date: Tue, 27 Aug 2024 15:56:42 +0200 Subject: [PATCH] skip chores on deleted vnodes --- packages/qwik/src/core/api.md | 10 +++--- .../qwik/src/core/render/dom/notify-render.ts | 2 +- packages/qwik/src/core/state/common.ts | 12 +++---- packages/qwik/src/core/use/use-task.ts | 2 +- .../qwik/src/core/v2/client/dom-container.ts | 2 +- .../qwik/src/core/v2/client/dom-render.ts | 4 +-- packages/qwik/src/core/v2/client/types.ts | 36 ++++++++++--------- .../qwik/src/core/v2/client/vnode-diff.ts | 22 +++++------- packages/qwik/src/core/v2/shared/scheduler.ts | 29 +++++++++------ .../src/core/v2/shared/scheduler.unit.tsx | 26 ++++++-------- packages/qwik/src/core/v2/signal/v2-signal.ts | 10 +++--- .../src/core/v2/signal/v2-signal.unit.tsx | 4 +-- .../src/core/v2/ssr/ssr-render-component.ts | 2 +- .../qwik/src/testing/rendering.unit-util.tsx | 2 +- 14 files changed, 82 insertions(+), 81 deletions(-) diff --git a/packages/qwik/src/core/api.md b/packages/qwik/src/core/api.md index f7cf8eede7d..5c09b347445 100644 --- a/packages/qwik/src/core/api.md +++ b/packages/qwik/src/core/api.md @@ -2041,6 +2041,8 @@ export type _VNode = _ElementVNode | _TextVNode | _VirtualVNode; // @internal export const enum _VNodeFlags { + // (undocumented) + Deleted = 32, // (undocumented) Element = 1, // (undocumented) @@ -2052,15 +2054,15 @@ export const enum _VNodeFlags { // (undocumented) INFLATED_TYPE_MASK = 15, // (undocumented) - NAMESPACE_MASK = 96, + NAMESPACE_MASK = 192, // (undocumented) - NEGATED_NAMESPACE_MASK = -97, + NEGATED_NAMESPACE_MASK = -193, // (undocumented) NS_html = 0, // (undocumented) - NS_math = 64, + NS_math = 128, // (undocumented) - NS_svg = 32, + NS_svg = 64, // (undocumented) Resolved = 16, // (undocumented) diff --git a/packages/qwik/src/core/render/dom/notify-render.ts b/packages/qwik/src/core/render/dom/notify-render.ts index 4d752f1a6e7..8644f69e2b4 100644 --- a/packages/qwik/src/core/render/dom/notify-render.ts +++ b/packages/qwik/src/core/render/dom/notify-render.ts @@ -137,7 +137,7 @@ export const _hW = () => { const [task] = useLexicalScope<[SubscriberEffect]>(); const container = getDomContainer(task.$el$ as Element); const type = task.$flags$ & TaskFlags.VISIBLE_TASK ? ChoreType.VISIBLE : ChoreType.TASK; - container.$scheduler$.schedule(type, task as Task); + container.$scheduler$(type, task as Task); }; const renderMarked = async (containerState: ContainerState): Promise => { diff --git a/packages/qwik/src/core/state/common.ts b/packages/qwik/src/core/state/common.ts index d83003379f0..128822c27ef 100644 --- a/packages/qwik/src/core/state/common.ts +++ b/packages/qwik/src/core/state/common.ts @@ -199,7 +199,7 @@ export const isConnected = (sub: SubscriberEffect | SubscriberHost): boolean => /** @public */ export const unwrapProxy = (proxy: T): T => { - return isObject(proxy) ? getProxyTarget(proxy) ?? proxy : proxy; + return isObject(proxy) ? (getProxyTarget(proxy) ?? proxy) : proxy; }; export const getProxyTarget = (obj: T): T | undefined => { @@ -496,10 +496,10 @@ export class LocalSubscriptionManager { if (isComputedTask(host)) { // scheduler(ChoreType.COMPUTED, host); } else if (isResourceTask(host)) { - scheduler.schedule(ChoreType.RESOURCE, host); + scheduler(ChoreType.RESOURCE, host); } else { const task = host as Task; - scheduler.schedule( + scheduler( task.$flags$ & TaskFlags.VISIBLE_TASK ? ChoreType.VISIBLE : ChoreType.TASK, task ); @@ -514,7 +514,7 @@ export class LocalSubscriptionManager { host as fixMeAny, ELEMENT_PROPS ); - scheduler.schedule(ChoreType.COMPONENT, host as fixMeAny, componentQrl, componentProps); + scheduler(ChoreType.COMPONENT, host as fixMeAny, componentQrl, componentProps); } } else { const signal = sub[SubscriptionProp.SIGNAL]; @@ -563,7 +563,7 @@ export class LocalSubscriptionManager { type == SubscriptionType.PROP_IMMUTABLE ); } else { - scheduler.schedule( + scheduler( ChoreType.NODE_DIFF, host as fixMeAny, sub[SubscriptionProp.ELEMENT] as fixMeAny, @@ -598,7 +598,7 @@ function updateNodeProp( const element = target[ElementVNodeProps.element] as Element; container.$journal$.push(VNodeJournalOpCode.SetAttribute, element, propKey, value); } - container.$scheduler$.schedule(ChoreType.JOURNAL_FLUSH); + container.$scheduler$(ChoreType.JOURNAL_FLUSH); } let __lastSubscription: Subscriptions | undefined; diff --git a/packages/qwik/src/core/use/use-task.ts b/packages/qwik/src/core/use/use-task.ts index ae15df71ae5..d1cb1b42924 100644 --- a/packages/qwik/src/core/use/use-task.ts +++ b/packages/qwik/src/core/use/use-task.ts @@ -473,7 +473,7 @@ export const useVisibleTaskQrl = (qrl: QRL, opts?: OnVisibleTaskOptions) useRunTask(task, eagerness); if (!isServerPlatform()) { qrl.$resolveLazy$(iCtx.$hostElement$ as fixMeAny); - iCtx.$container2$.$scheduler$.schedule(ChoreType.VISIBLE, task); + iCtx.$container2$.$scheduler$(ChoreType.VISIBLE, task); } } else { const task = new Task(TaskFlags.VISIBLE_TASK, i, elCtx.$element$, qrl, undefined, null); diff --git a/packages/qwik/src/core/v2/client/dom-container.ts b/packages/qwik/src/core/v2/client/dom-container.ts index 54a882f1121..e69a1e9730a 100644 --- a/packages/qwik/src/core/v2/client/dom-container.ts +++ b/packages/qwik/src/core/v2/client/dom-container.ts @@ -278,7 +278,7 @@ export class DomContainer extends _SharedContainer implements IClientContainer, this.rendering = true; this.renderDone = getPlatform().nextTick(() => { // console.log('>>>> scheduleRender nextTick', !!this.rendering); - return maybeThen(this.$scheduler$.schedule(ChoreType.WAIT_FOR_ALL), () => { + return maybeThen(this.$scheduler$(ChoreType.WAIT_FOR_ALL), () => { // console.log('>>>> scheduleRender done', !!this.rendering); this.rendering = false; }); diff --git a/packages/qwik/src/core/v2/client/dom-render.ts b/packages/qwik/src/core/v2/client/dom-render.ts index a0fb37a01d0..00330413218 100644 --- a/packages/qwik/src/core/v2/client/dom-render.ts +++ b/packages/qwik/src/core/v2/client/dom-render.ts @@ -42,8 +42,8 @@ export const render2 = async ( const container = getDomContainer(parent as HTMLElement) as DomContainer; container.$serverData$ = opts.serverData!; const host: HostElement = container.rootVNode as fixMeAny; - container.$scheduler$.schedule(ChoreType.NODE_DIFF, host, host, jsxNode as JSXNode); - await container.$scheduler$.schedule(ChoreType.WAIT_FOR_ALL); + container.$scheduler$(ChoreType.NODE_DIFF, host, host, jsxNode as JSXNode); + await container.$scheduler$(ChoreType.WAIT_FOR_ALL); return { cleanup: () => { cleanup(container, container.rootVNode); diff --git a/packages/qwik/src/core/v2/client/types.ts b/packages/qwik/src/core/v2/client/types.ts index 1730c5b97b9..2ca2bbd497d 100644 --- a/packages/qwik/src/core/v2/client/types.ts +++ b/packages/qwik/src/core/v2/client/types.ts @@ -80,29 +80,31 @@ export interface QNode extends Node { * @internal */ export const enum VNodeFlags { - Element /* ****************** */ = 0b00_00001, - Virtual /* ****************** */ = 0b00_00010, - ELEMENT_OR_VIRTUAL_MASK /* ** */ = 0b00_00011, - ELEMENT_OR_TEXT_MASK /* ***** */ = 0b00_00101, - TYPE_MASK /* **************** */ = 0b00_00111, - INFLATED_TYPE_MASK /* ******* */ = 0b00_01111, - Text /* ********************* */ = 0b00_00100, + Element /* ****************** */ = 0b00_000001, + Virtual /* ****************** */ = 0b00_000010, + ELEMENT_OR_VIRTUAL_MASK /* ** */ = 0b00_000011, + ELEMENT_OR_TEXT_MASK /* ***** */ = 0b00_000101, + TYPE_MASK /* **************** */ = 0b00_000111, + INFLATED_TYPE_MASK /* ******* */ = 0b00_001111, + Text /* ********************* */ = 0b00_000100, /// Extra flag which marks if a node needs to be inflated. - Inflated /* ***************** */ = 0b00_01000, + Inflated /* ***************** */ = 0b00_001000, /// Marks if the `ensureProjectionResolved` has been called on the node. - Resolved /* ***************** */ = 0b00_10000, + Resolved /* ***************** */ = 0b00_010000, + /// Marks if the vnode is deleted. + Deleted /* ****************** */ = 0b00_100000, /// Flags for Namespace - NAMESPACE_MASK /* *********** */ = 0b11_00000, - NEGATED_NAMESPACE_MASK /* ** */ = ~0b11_00000, - NS_html /* ****************** */ = 0b00_00000, // http://www.w3.org/1999/xhtml - NS_svg /* ******************* */ = 0b01_00000, // http://www.w3.org/2000/svg - NS_math /* ****************** */ = 0b10_00000, // http://www.w3.org/1998/Math/MathML + NAMESPACE_MASK /* *********** */ = 0b11_000000, + NEGATED_NAMESPACE_MASK /* ** */ = ~0b11_000000, + NS_html /* ****************** */ = 0b00_000000, // http://www.w3.org/1999/xhtml + NS_svg /* ******************* */ = 0b01_000000, // http://www.w3.org/2000/svg + NS_math /* ****************** */ = 0b10_000000, // http://www.w3.org/1998/Math/MathML } export const enum VNodeFlagsIndex { - mask /* ************* */ = ~0b11_11111, - negated_mask /* ****** */ = 0b11_11111, - shift /* ************* */ = 7, + mask /* ************* */ = ~0b11_111111, + negated_mask /* ****** */ = 0b11_111111, + shift /* ************* */ = 8, } export const enum VNodeProps { diff --git a/packages/qwik/src/core/v2/client/vnode-diff.ts b/packages/qwik/src/core/v2/client/vnode-diff.ts index 0b403b434c4..4a02ce45905 100644 --- a/packages/qwik/src/core/v2/client/vnode-diff.ts +++ b/packages/qwik/src/core/v2/client/vnode-diff.ts @@ -1019,7 +1019,7 @@ export const vnode_diff = ( const vNodeProps = vnode_getProp(host, ELEMENT_PROPS, container.$getObjectById$); shouldRender = shouldRender || propsDiffer(jsxProps, vNodeProps); if (shouldRender) { - container.$scheduler$.schedule(ChoreType.COMPONENT, host, componentQRL, jsxProps); + container.$scheduler$(ChoreType.COMPONENT, host, componentQRL, jsxProps); } } jsxValue.children != null && descendContentToProject(jsxValue.children, host); @@ -1213,7 +1213,7 @@ export function cleanup(container: ClientContainer, vNode: VNode) { if (type & VNodeFlags.Virtual) { // Only virtual nodes have subscriptions clearVNodeDependencies(vCursor); - cleanupVNodeChores(vNode, vParent, vCursor, container); + markVNodeAsDeleted(vNode, vParent, vCursor); const seq = container.getHostProp>(vCursor as VirtualVNode, ELEMENT_SEQ); if (seq) { for (let i = 0; i < seq.length; i++) { @@ -1222,7 +1222,7 @@ export function cleanup(container: ClientContainer, vNode: VNode) { const task = obj; clearSubscriberDependencies(task); if (obj.$flags$ & TaskFlags.VISIBLE_TASK) { - container.$scheduler$.schedule(ChoreType.CLEANUP_VISIBLE, obj); + container.$scheduler$(ChoreType.CLEANUP_VISIBLE, obj); } else { cleanupTask(task); } @@ -1331,25 +1331,19 @@ function cleanupStaleUnclaimedProjection(journal: VNodeJournal, projection: VNod } } -function cleanupVNodeChores( - vNode: VNode, - vParent: VNode | null, - vCursor: VNode, - container: ClientContainer -) { +function markVNodeAsDeleted(vNode: VNode, vParent: VNode | null, vCursor: VNode) { /** - * Cleanup stale chores for current vCursor, but only if it is not a projection. We need to do - * this to prevent stale chores from running after the vnode is removed. (for example signal - * subscriptions) + * Marks vCursor as deleted, but only if it is not a projection. We need to do this to prevent + * chores from running after the vnode is removed. (for example signal subscriptions) */ if (vNode !== vCursor) { - container.$scheduler$.cleanupStaleChores(vCursor as VirtualVNode); + vCursor[VNodeProps.flags] |= VNodeFlags.Deleted; } else { const currentVParent = vParent || vnode_getParent(vNode); const isParentProjection = currentVParent && vnode_getProp(currentVParent, QSlot, null) !== null; if (!isParentProjection) { - container.$scheduler$.cleanupStaleChores(vCursor as VirtualVNode); + vCursor[VNodeProps.flags] |= VNodeFlags.Deleted; } } } diff --git a/packages/qwik/src/core/v2/shared/scheduler.ts b/packages/qwik/src/core/v2/shared/scheduler.ts index 1692d99549d..4a12750df4a 100644 --- a/packages/qwik/src/core/v2/shared/scheduler.ts +++ b/packages/qwik/src/core/v2/shared/scheduler.ts @@ -97,7 +97,13 @@ import { logWarn } from '../../util/log'; import { isPromise, maybeThen, maybeThenPassError, safeCall } from '../../util/promises'; import type { ValueOrPromise } from '../../util/types'; import { isDomContainer } from '../client/dom-container'; -import { ElementVNodeProps, type ElementVNode, type VirtualVNode } from '../client/types'; +import { + ElementVNodeProps, + VNodeFlags, + VNodeProps, + type ElementVNode, + type VirtualVNode, +} from '../client/types'; import { vnode_documentPosition, vnode_isVNode, @@ -163,7 +169,7 @@ export const createScheduler = ( let currentChore: Chore | null = null; let journalFlushScheduled: boolean = false; - return { schedule, cleanupStaleChores }; + return schedule; //////////////////////////////////////////////////////////////////////////////// //////////////////////////////////////////////////////////////////////////////// @@ -283,6 +289,17 @@ export const createScheduler = ( // we have processed all of the chores up to the given chore. break; } + const isDeletedVNode = + nextChore.$host$ && + vnode_isVNode(nextChore.$host$) && + nextChore.$host$[VNodeProps.flags] & VNodeFlags.Deleted; + if ( + isDeletedVNode && + // we need to process cleanup tasks for deleted nodes + nextChore.$type$ !== ChoreType.CLEANUP_VISIBLE + ) { + continue; + } const returnValue = executeChore(nextChore); if (isPromise(returnValue)) { return returnValue.then(() => drainUpTo(runUptoChore)); @@ -380,14 +397,6 @@ export const createScheduler = ( return (chore.$returnValue$ = value); }); } - - function cleanupStaleChores(host: HostElement): void { - for (let i = choreQueue.length - 1; i >= 0; i--) { - if (choreQueue[i].$host$ === host) { - choreQueue.splice(i, 1); - } - } - } }; const toNumber = (value: number | string): number => { diff --git a/packages/qwik/src/core/v2/shared/scheduler.unit.tsx b/packages/qwik/src/core/v2/shared/scheduler.unit.tsx index 2c8c81eb1c3..86af2949e2e 100644 --- a/packages/qwik/src/core/v2/shared/scheduler.unit.tsx +++ b/packages/qwik/src/core/v2/shared/scheduler.unit.tsx @@ -54,13 +54,10 @@ describe('scheduler', () => { }); it('should execute sort tasks', async () => { - scheduler.schedule( - ChoreType.TASK, - mockTask(vBHost1, { index: 2, qrl: $(() => testLog.push('b1.2')) }) - ); - scheduler.schedule(ChoreType.TASK, mockTask(vAHost, { qrl: $(() => testLog.push('a1')) })); - scheduler.schedule(ChoreType.TASK, mockTask(vBHost1, { qrl: $(() => testLog.push('b1.0')) })); - await scheduler.schedule(ChoreType.WAIT_FOR_ALL); + scheduler(ChoreType.TASK, mockTask(vBHost1, { index: 2, qrl: $(() => testLog.push('b1.2')) })); + scheduler(ChoreType.TASK, mockTask(vAHost, { qrl: $(() => testLog.push('a1')) })); + scheduler(ChoreType.TASK, mockTask(vBHost1, { qrl: $(() => testLog.push('b1.0')) })); + await scheduler(ChoreType.WAIT_FOR_ALL); expect(testLog).toEqual([ 'a1', // DepthFirst a host component is before b host component. 'b1.0', // Same component but smaller index. @@ -69,15 +66,12 @@ describe('scheduler', () => { ]); }); it('should execute visible tasks after journal flush', async () => { - scheduler.schedule( + scheduler( ChoreType.TASK, mockTask(vBHost2, { index: 2, qrl: $(() => testLog.push('b2.2: Task')) }) ); - scheduler.schedule( - ChoreType.TASK, - mockTask(vBHost1, { qrl: $(() => testLog.push('b1.0: Task')) }) - ); - scheduler.schedule( + scheduler(ChoreType.TASK, mockTask(vBHost1, { qrl: $(() => testLog.push('b1.0: Task')) })); + scheduler( ChoreType.VISIBLE, mockTask(vBHost2, { index: 2, @@ -85,7 +79,7 @@ describe('scheduler', () => { visible: true, }) ); - scheduler.schedule( + scheduler( ChoreType.VISIBLE, mockTask(vBHost1, { qrl: $(() => { @@ -94,13 +88,13 @@ describe('scheduler', () => { visible: true, }) ); - scheduler.schedule( + scheduler( ChoreType.COMPONENT, vBHost1, $(() => testLog.push('b1: Render')), {} ); - await scheduler.schedule(ChoreType.WAIT_FOR_ALL); + await scheduler(ChoreType.WAIT_FOR_ALL); expect(testLog).toEqual([ 'b1.0: Task', 'b1: Render', diff --git a/packages/qwik/src/core/v2/signal/v2-signal.ts b/packages/qwik/src/core/v2/signal/v2-signal.ts index ab3c2d1cbfa..73471216fee 100644 --- a/packages/qwik/src/core/v2/signal/v2-signal.ts +++ b/packages/qwik/src/core/v2/signal/v2-signal.ts @@ -301,7 +301,7 @@ export const triggerEffects = ( } else if (effect.$flags$ & TaskFlags.RESOURCE) { choreType = ChoreType.RESOURCE; } - container.$scheduler$.schedule(choreType, effect); + container.$scheduler$(choreType, effect); } else if (effect instanceof Signal2) { // we don't schedule ComputedSignal/DerivedSignal directly, instead we invalidate it and // and schedule the signals effects (recursively) @@ -309,7 +309,7 @@ export const triggerEffects = ( // Ensure that the computed signal's QRL is resolved. // If not resolved schedule it to be resolved. if (!effect.$computeQrl$.resolved) { - container.$scheduler$.schedule(ChoreType.QRL_RESOLVE, null, effect.$computeQrl$); + container.$scheduler$(ChoreType.QRL_RESOLVE, null, effect.$computeQrl$); } } (effect as ComputedSignal2 | DerivedSignal2).$invalid$ = true; @@ -327,11 +327,11 @@ export const triggerEffects = ( const qrl = container.getHostProp any>>(host, OnRenderProp); assertDefined(qrl, 'Component must have QRL'); const props = container.getHostProp(host, ELEMENT_PROPS); - container.$scheduler$.schedule(ChoreType.COMPONENT, host, qrl, props); + container.$scheduler$(ChoreType.COMPONENT, host, qrl, props); } else if (property === EffectProperty.VNODE) { const host: HostElement = effect as any; const target = host; - container.$scheduler$.schedule(ChoreType.NODE_DIFF, host, target, signal as fixMeAny); + container.$scheduler$(ChoreType.NODE_DIFF, host, target, signal as fixMeAny); } else { const host: HostElement = effect as any; const scopedStyleIdPrefix: string | null = @@ -340,7 +340,7 @@ export const triggerEffects = ( value: signal, scopedStyleIdPrefix, }; - container.$scheduler$.schedule(ChoreType.NODE_PROP, host, property, payload); + container.$scheduler$(ChoreType.NODE_PROP, host, property, payload); } }; effects.forEach(scheduleEffect); diff --git a/packages/qwik/src/core/v2/signal/v2-signal.unit.tsx b/packages/qwik/src/core/v2/signal/v2-signal.unit.tsx index fb77f3da372..701fa8d5a2b 100644 --- a/packages/qwik/src/core/v2/signal/v2-signal.unit.tsx +++ b/packages/qwik/src/core/v2/signal/v2-signal.unit.tsx @@ -27,7 +27,7 @@ describe('v2-signal', () => { afterEach(async () => { delayMap.clear(); - await container.$scheduler$.schedule(ChoreType.WAIT_FOR_ALL); + await container.$scheduler$(ChoreType.WAIT_FOR_ALL); await getTestPlatform().flush(); container = null!; }); @@ -134,7 +134,7 @@ describe('v2-signal', () => { } function flushSignals() { - return container.$scheduler$.schedule(ChoreType.WAIT_FOR_ALL); + return container.$scheduler$(ChoreType.WAIT_FOR_ALL); } /** Simulates the QRLs being lazy loaded once per test. */ diff --git a/packages/qwik/src/core/v2/ssr/ssr-render-component.ts b/packages/qwik/src/core/v2/ssr/ssr-render-component.ts index b406577fdc3..2b5b7c689d3 100644 --- a/packages/qwik/src/core/v2/ssr/ssr-render-component.ts +++ b/packages/qwik/src/core/v2/ssr/ssr-render-component.ts @@ -33,5 +33,5 @@ export const applyQwikComponentBody = ( if (jsx.key !== null) { host.setProp(ELEMENT_KEY, jsx.key); } - return scheduler.schedule(ChoreType.COMPONENT_SSR, host, componentQrl, srcProps); + return scheduler(ChoreType.COMPONENT_SSR, host, componentQrl, srcProps); }; diff --git a/packages/qwik/src/testing/rendering.unit-util.tsx b/packages/qwik/src/testing/rendering.unit-util.tsx index 6efce6091a0..57ceed79fec 100644 --- a/packages/qwik/src/testing/rendering.unit-util.tsx +++ b/packages/qwik/src/testing/rendering.unit-util.tsx @@ -197,7 +197,7 @@ export async function rerenderComponent(element: HTMLElement) { const host = getHostVNode(vElement)!; const qrl = container.getHostProp>>(host, OnRenderProp)!; const props = container.getHostProp(host, ELEMENT_PROPS); - await container.$scheduler$.schedule(ChoreType.COMPONENT, host, qrl, props); + await container.$scheduler$(ChoreType.COMPONENT, host, qrl, props); await getTestPlatform().flush(); }