fix not execute signal when not used

QwikDev · Aug 18, 2024 · 5c1424c · 5c1424c
1 parent a8d830e
commit 5c1424c
Show file tree

Hide file tree

Showing 13 changed files with 59 additions and 37 deletions.
diff --git 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<void> => {

diff --git 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
@@ -488,7 +488,7 @@ export const useVisibleTaskQrl = (qrl: QRL<TaskFn>, 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
@@ -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
@@ -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
@@ -1014,7 +1014,7 @@ export const vnode_diff = (
         const vNodeProps = vnode_getProp<any>(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
@@ -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
@@ -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,20 +69,23 @@ 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,
         qrl: $(() => testLog.push('b2.2: VisibleTask')),
         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
@@ -291,15 +291,15 @@ 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)
         if (effect instanceof ComputedSignal2) {
           // 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<unknown> | DerivedSignal2<unknown>).$invalid$ = true;
@@ -315,14 +315,14 @@ export const triggerEffects = (
         const qrl = container.getHostProp<QRL<(...args: any[]) => any>>(host, OnRenderProp);
         assertDefined(qrl, 'Component must have QRL');
         const props = container.getHostProp<any>(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
@@ -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
@@ -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
@@ -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
@@ -197,7 +197,7 @@ export async function rerenderComponent(element: HTMLElement) {
   const host = getHostVNode(vElement)!;
   const qrl = container.getHostProp<QRL<OnRenderFn<any>>>(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();
 }