From 5c1424c574c62151d2dc993c3240f6b58b6a7db8 Mon Sep 17 00:00:00 2001 From: Varixo Date: Sun, 18 Aug 2024 19:14:02 +0200 Subject: [PATCH] fix not execute signal when not used --- .../qwik/src/core/render/dom/notify-render.ts | 2 +- packages/qwik/src/core/state/common.ts | 10 +++---- packages/qwik/src/core/use/use-task.ts | 2 +- .../qwik/src/core/v2/client/dom-container.ts | 7 +++-- .../qwik/src/core/v2/client/dom-render.ts | 4 +-- .../qwik/src/core/v2/client/vnode-diff.ts | 14 +++++++--- packages/qwik/src/core/v2/shared/scheduler.ts | 10 ++++++- .../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 +- .../src/core/v2/tests/use-signal.spec.tsx | 3 +-- .../qwik/src/testing/rendering.unit-util.tsx | 2 +- 13 files changed, 59 insertions(+), 37 deletions(-) diff --git a/packages/qwik/src/core/render/dom/notify-render.ts b/packages/qwik/src/core/render/dom/notify-render.ts index 8644f69e2b4..4d752f1a6e7 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$(type, task as Task); + container.$scheduler$.schedule(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 bb7d5fbc3ee..d83003379f0 100644 --- a/packages/qwik/src/core/state/common.ts +++ b/packages/qwik/src/core/state/common.ts @@ -496,10 +496,10 @@ export class LocalSubscriptionManager { if (isComputedTask(host)) { // scheduler(ChoreType.COMPUTED, host); } else if (isResourceTask(host)) { - scheduler(ChoreType.RESOURCE, host); + scheduler.schedule(ChoreType.RESOURCE, host); } else { const task = host as Task; - scheduler( + scheduler.schedule( task.$flags$ & TaskFlags.VISIBLE_TASK ? ChoreType.VISIBLE : ChoreType.TASK, task ); @@ -514,7 +514,7 @@ export class LocalSubscriptionManager { host as fixMeAny, ELEMENT_PROPS ); - scheduler(ChoreType.COMPONENT, host as fixMeAny, componentQrl, componentProps); + scheduler.schedule(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( + scheduler.schedule( 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$(ChoreType.JOURNAL_FLUSH); + container.$scheduler$.schedule(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 3c3847ace0e..0b7a92e7b9d 100644 --- a/packages/qwik/src/core/use/use-task.ts +++ b/packages/qwik/src/core/use/use-task.ts @@ -488,7 +488,7 @@ export const useVisibleTaskQrl = (qrl: QRL, opts?: OnVisibleTaskOptions) useRunTask(task, eagerness); if (!isServerPlatform()) { qrl.$resolveLazy$(iCtx.$hostElement$ as fixMeAny); - iCtx.$container2$.$scheduler$(ChoreType.VISIBLE, task); + iCtx.$container2$.$scheduler$.schedule(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 0b7a5095a5d..54a882f1121 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$(ChoreType.WAIT_FOR_ALL), () => { + return maybeThen(this.$scheduler$.schedule(ChoreType.WAIT_FOR_ALL), () => { // console.log('>>>> scheduleRender done', !!this.rendering); this.rendering = false; }); @@ -306,7 +306,10 @@ export class DomContainer extends _SharedContainer implements IClientContainer, if (typeof id === 'string') { id = parseFloat(id); } - assertTrue(id < this.$rawStateData$.length, `Invalid reference: ${id} < ${this.$rawStateData$.length}`); + assertTrue( + id < this.$rawStateData$.length, + `Invalid reference: ${id} < ${this.$rawStateData$.length}` + ); return this.stateData[id]; }; diff --git a/packages/qwik/src/core/v2/client/dom-render.ts b/packages/qwik/src/core/v2/client/dom-render.ts index 00330413218..a0fb37a01d0 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$(ChoreType.NODE_DIFF, host, host, jsxNode as JSXNode); - await container.$scheduler$(ChoreType.WAIT_FOR_ALL); + container.$scheduler$.schedule(ChoreType.NODE_DIFF, host, host, jsxNode as JSXNode); + await container.$scheduler$.schedule(ChoreType.WAIT_FOR_ALL); return { cleanup: () => { cleanup(container, container.rootVNode); diff --git a/packages/qwik/src/core/v2/client/vnode-diff.ts b/packages/qwik/src/core/v2/client/vnode-diff.ts index 72563f5d663..722832b620d 100644 --- a/packages/qwik/src/core/v2/client/vnode-diff.ts +++ b/packages/qwik/src/core/v2/client/vnode-diff.ts @@ -1014,7 +1014,7 @@ export const vnode_diff = ( const vNodeProps = vnode_getProp(host, ELEMENT_PROPS, container.$getObjectById$); shouldRender = shouldRender || propsDiffer(jsxProps, vNodeProps); if (shouldRender) { - container.$scheduler$(ChoreType.COMPONENT, host, componentQRL, jsxProps); + container.$scheduler$.schedule(ChoreType.COMPONENT, host, componentQRL, jsxProps); } } jsxValue.children != null && descendContentToProject(jsxValue.children, host); @@ -1216,7 +1216,7 @@ export function cleanup(container: ClientContainer, vNode: VNode) { const task = obj; clearSubscriberDependencies(task); if (obj.$flags$ & TaskFlags.VISIBLE_TASK) { - container.$scheduler$(ChoreType.CLEANUP_VISIBLE, obj); + container.$scheduler$.schedule(ChoreType.CLEANUP_VISIBLE, obj); } else { cleanupTask(task); } @@ -1253,14 +1253,20 @@ export function cleanup(container: ClientContainer, vNode: VNode) { } } - const isSlot = + const isProjection = type & VNodeFlags.Virtual && vnode_getProp(vCursor as VirtualVNode, QSlot, null) !== null; // Descend into children - if (!isSlot) { + if (!isProjection) { // Only if it is not a projection const vFirstChild = vnode_getFirstChild(vCursor); if (vFirstChild) { vCursor = vFirstChild; + /** + * 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) + */ + container.$scheduler$.cleanupStaleChores(vCursor as VirtualVNode); continue; } } else if (vCursor === vNode) { diff --git a/packages/qwik/src/core/v2/shared/scheduler.ts b/packages/qwik/src/core/v2/shared/scheduler.ts index 183e6848b30..f891230b1c1 100644 --- a/packages/qwik/src/core/v2/shared/scheduler.ts +++ b/packages/qwik/src/core/v2/shared/scheduler.ts @@ -153,7 +153,7 @@ export const createScheduler = ( let currentChore: Chore | null = null; let journalFlushScheduled: boolean = false; - return schedule; + return { schedule, cleanupStaleChores }; //////////////////////////////////////////////////////////////////////////////// //////////////////////////////////////////////////////////////////////////////// @@ -361,6 +361,14 @@ 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 86af2949e2e..2c8c81eb1c3 100644 --- a/packages/qwik/src/core/v2/shared/scheduler.unit.tsx +++ b/packages/qwik/src/core/v2/shared/scheduler.unit.tsx @@ -54,10 +54,13 @@ describe('scheduler', () => { }); it('should execute sort tasks', async () => { - 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); + 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); expect(testLog).toEqual([ 'a1', // DepthFirst a host component is before b host component. 'b1.0', // Same component but smaller index. @@ -66,12 +69,15 @@ describe('scheduler', () => { ]); }); it('should execute visible tasks after journal flush', async () => { - scheduler( + scheduler.schedule( ChoreType.TASK, mockTask(vBHost2, { index: 2, qrl: $(() => testLog.push('b2.2: Task')) }) ); - scheduler(ChoreType.TASK, mockTask(vBHost1, { qrl: $(() => testLog.push('b1.0: Task')) })); - scheduler( + scheduler.schedule( + ChoreType.TASK, + mockTask(vBHost1, { qrl: $(() => testLog.push('b1.0: Task')) }) + ); + scheduler.schedule( ChoreType.VISIBLE, mockTask(vBHost2, { index: 2, @@ -79,7 +85,7 @@ describe('scheduler', () => { visible: true, }) ); - scheduler( + scheduler.schedule( ChoreType.VISIBLE, mockTask(vBHost1, { qrl: $(() => { @@ -88,13 +94,13 @@ describe('scheduler', () => { visible: true, }) ); - scheduler( + scheduler.schedule( ChoreType.COMPONENT, vBHost1, $(() => testLog.push('b1: Render')), {} ); - await scheduler(ChoreType.WAIT_FOR_ALL); + await scheduler.schedule(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 76a992abc9f..aa391d9d218 100644 --- a/packages/qwik/src/core/v2/signal/v2-signal.ts +++ b/packages/qwik/src/core/v2/signal/v2-signal.ts @@ -291,7 +291,7 @@ export const triggerEffects = ( } else if (effect.$flags$ & TaskFlags.RESOURCE) { choreType = ChoreType.RESOURCE; } - container.$scheduler$(choreType, effect); + container.$scheduler$.schedule(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) @@ -299,7 +299,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$(ChoreType.QRL_RESOLVE, null, effect.$computeQrl$); + container.$scheduler$.schedule(ChoreType.QRL_RESOLVE, null, effect.$computeQrl$); } } (effect as ComputedSignal2 | DerivedSignal2).$invalid$ = true; @@ -315,14 +315,14 @@ 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$(ChoreType.COMPONENT, host, qrl, props); + container.$scheduler$.schedule(ChoreType.COMPONENT, host, qrl, props); } else if (property === EffectProperty.VNODE) { const host: HostElement = effect as any; const target = host; - container.$scheduler$(ChoreType.NODE_DIFF, host, target, signal as fixMeAny); + container.$scheduler$.schedule(ChoreType.NODE_DIFF, host, target, signal as fixMeAny); } else { const host: HostElement = effect as any; - container.$scheduler$(ChoreType.NODE_PROP, host, property, signal as fixMeAny); + container.$scheduler$.schedule(ChoreType.NODE_PROP, host, property, signal as fixMeAny); } }; 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 20519db33af..935e68342f7 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$(ChoreType.WAIT_FOR_ALL); + await container.$scheduler$.schedule(ChoreType.WAIT_FOR_ALL); await getTestPlatform().flush(); container = null!; }); @@ -134,7 +134,7 @@ describe('v2-signal', () => { } function flushSignals() { - return container.$scheduler$(ChoreType.WAIT_FOR_ALL); + return container.$scheduler$.schedule(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 2b5b7c689d3..b406577fdc3 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(ChoreType.COMPONENT_SSR, host, componentQrl, srcProps); + return scheduler.schedule(ChoreType.COMPONENT_SSR, host, componentQrl, srcProps); }; diff --git a/packages/qwik/src/core/v2/tests/use-signal.spec.tsx b/packages/qwik/src/core/v2/tests/use-signal.spec.tsx index 2d62100c00c..7819f6e8df4 100644 --- a/packages/qwik/src/core/v2/tests/use-signal.spec.tsx +++ b/packages/qwik/src/core/v2/tests/use-signal.spec.tsx @@ -227,8 +227,7 @@ describe.each([ ); }); - // TODO: should be fixed after signals v2 is implemented - it.skip('should not execute signal when not used', async () => { + it('should not execute signal when not used', async () => { const Cmp = component$(() => { const data = useSignal<{ price: number } | null>({ price: 100 }); return ( diff --git a/packages/qwik/src/testing/rendering.unit-util.tsx b/packages/qwik/src/testing/rendering.unit-util.tsx index 57ceed79fec..6efce6091a0 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$(ChoreType.COMPONENT, host, qrl, props); + await container.$scheduler$.schedule(ChoreType.COMPONENT, host, qrl, props); await getTestPlatform().flush(); }