Merge pull request #6996 from QwikDev/v2-fix-4918

fix: should not trigger effects if computed value is not changed

.changeset/config.json

@@ -7,14 +7,7 @@
   "access": "public",
   "baseBranch": "build/v2",
   "updateInternalDependencies": "minor",
-  "ignore": [
-    "qwik-docs",
-    "@qwik.dev/labs",
-    "insights",
-    "@qwik.dev/react",
-    "@qwik.dev/worker",
-    "qwik-cli-e2e"
-  ],
+  "ignore": ["qwik-docs", "insights", "@qwik.dev/react", "qwik-cli-e2e"],
   "___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH": {
     "onlyUpdatePeerDependentsWhenOutOfRange": true
   }

.changeset/rotten-weeks-tickle.md

@@ -0,0 +1,5 @@
+---
+'@qwik.dev/core': patch
+---
+
+fix: do not trigger effects if computed value is not changed

packages/qwik/src/core/shared/utils/promises.ts

@@ -1,6 +1,8 @@
 import { throwErrorAndStop } from './log';
 import type { ValueOrPromise } from './types';
 
+export const MAX_RETRY_ON_PROMISE_COUNT = 10;
+
 export type PromiseTree<T> = T | Promise<T> | Promise<T[]> | Array<PromiseTree<T>>;
 
 export const isPromise = (value: any): value is Promise<any> => {
@@ -103,3 +105,14 @@ export const delay = (timeout: number) => {
     setTimeout(resolve, timeout);
   });
 };
+
+export function retryOnPromise<T>(fn: () => T, retryCount: number = 0): ValueOrPromise<T> {
+  try {
+    return fn();
+  } catch (e) {
+    if (isPromise(e) && retryCount < MAX_RETRY_ON_PROMISE_COUNT) {
+      return e.then(retryOnPromise.bind(null, fn, retryCount++)) as ValueOrPromise<T>;
+    }
+    throw e;
+  }
+}

packages/qwik/src/core/signal/signal.ts

@@ -19,7 +19,7 @@ import { trackSignal, tryGetInvokeContext } from '../use/use-core';
 import { Task, TaskFlags, isTask } from '../use/use-task';
 import { logError, throwErrorAndStop } from '../shared/utils/log';
 import { ELEMENT_PROPS, OnRenderProp, QSubscribers } from '../shared/utils/markers';
-import { isPromise } from '../shared/utils/promises';
+import { isPromise, retryOnPromise } from '../shared/utils/promises';
 import { qDev } from '../shared/utils/qdev';
 import type { VNode } from '../client/types';
 import { vnode_getProp, vnode_isVirtualVNode, vnode_isVNode, vnode_setProp } from '../client/vnode';
@@ -345,15 +345,12 @@ export const triggerEffects = (
             container.$scheduler$(ChoreType.QRL_RESOLVE, null, effect.$computeQrl$);
           }
         }
-        (effect as ComputedSignal<unknown> | WrappedSignal<unknown>).$invalid$ = true;
-        const previousSignal = signal;
         try {
-          signal = effect;
-          effect.$effects$?.forEach(scheduleEffect);
-        } catch (e: unknown) {
+          retryOnPromise(() =>
+            (effect as ComputedSignal<unknown> | WrappedSignal<unknown>).$invalidate$()
+          );
+        } catch (e) {
           logError(e);
-        } finally {
-          signal = previousSignal;
         }
       } else if (property === EffectProperty.COMPONENT) {
         const host: HostElement = effect as any;
@@ -458,7 +455,9 @@ export class ComputedSignal<T> extends Signal<T> {
       this.$invalid$ = false;
 
       const didChange = untrackedValue !== this.$untrackedValue$;
-      this.$untrackedValue$ = untrackedValue;
+      if (didChange) {
+        this.$untrackedValue$ = untrackedValue;
+      }
       return didChange;
     } finally {
       if (ctx) {
@@ -531,12 +530,17 @@ export class WrappedSignal<T> extends Signal<T> implements Subscriber {
     if (!this.$invalid$) {
       return false;
     }
-    this.$untrackedValue$ = trackSignal(
+    const untrackedValue = trackSignal(
       () => this.$func$(...this.$args$),
       this,
       EffectProperty.VNODE,
       this.$container$!
     );
+    const didChange = untrackedValue !== this.$untrackedValue$;
+    if (didChange) {
+      this.$untrackedValue$ = untrackedValue;
+    }
+    return didChange;
   }
 
   // Getters don't get inherited

packages/qwik/src/core/tests/use-computed.spec.tsx

@@ -5,11 +5,13 @@ import {
   createSignal,
   noSerialize,
   qrl,
+  untrack,
   useComputed$,
   useComputedQrl,
   useLexicalScope,
   useSignal,
   useStore,
+  useTask$,
 } from '@qwik.dev/core';
 import { domRender, ssrRenderToDom, trigger } from '@qwik.dev/core/testing';
 import { describe, expect, it } from 'vitest';
@@ -349,6 +351,37 @@ describe.each([
       );
       expect((globalThis as any).useComputedCount).toBe(1);
     });
+
+    it('#4918 - should not trigger track fn if value is not changed', async () => {
+      (globalThis as any).logCount = 0;
+      const Issue4918 = component$(() => {
+        const countRef = useSignal(0);
+        const isGreetOneRef = useComputed$(() => {
+          return countRef.value > 1;
+        });
+        useTask$(({ track }) => {
+          track(isGreetOneRef);
+          (globalThis as any).logCount++;
+        });
+        return (
+          <div>
+            <button onClick$={() => (countRef.value = countRef.value + 1)}>incr</button>
+          </div>
+        );
+      });
+
+      const { document } = await render(<Issue4918 />, { debug });
+      expect((globalThis as any).logCount).toBe(1);
+
+      await trigger(document.body, 'button', 'click');
+      expect((globalThis as any).logCount).toBe(1);
+      await trigger(document.body, 'button', 'click');
+      expect((globalThis as any).logCount).toBe(2);
+      await trigger(document.body, 'button', 'click');
+      expect((globalThis as any).logCount).toBe(2);
+      await trigger(document.body, 'button', 'click');
+      expect((globalThis as any).logCount).toBe(2);
+    });
   });
 
   it('should mark noSerialize as invalid after deserialization', async () => {
@@ -357,7 +390,7 @@ describe.each([
       const runCount = useSignal(0);
       const showCount = useSignal(false);
       const doubleCount = useComputed$(() => {
-        runCount.value++;
+        untrack(() => runCount.value++);
         return noSerialize({ double: count.value * 2 });
       });
       return (