From a2a504d5af3bd73d98367f343b60dba65534b239 Mon Sep 17 00:00:00 2001 From: Varixo Date: Wed, 23 Oct 2024 16:51:53 +0200 Subject: [PATCH 1/3] fix: should not trigger effects if computed value is not changed --- .../qwik/src/core/shared/utils/promises.ts | 13 +++++++ packages/qwik/src/core/signal/signal.ts | 28 ++++++++------- .../qwik/src/core/tests/use-computed.spec.tsx | 35 ++++++++++++++++++- 3 files changed, 63 insertions(+), 13 deletions(-) diff --git a/packages/qwik/src/core/shared/utils/promises.ts b/packages/qwik/src/core/shared/utils/promises.ts index 18b65776716..02e97f9561b 100644 --- a/packages/qwik/src/core/shared/utils/promises.ts +++ b/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 | Promise | Promise | Array>; export const isPromise = (value: any): value is Promise => { @@ -103,3 +105,14 @@ export const delay = (timeout: number) => { setTimeout(resolve, timeout); }); }; + +export function retryOnPromise(fn: () => T, retryCount: number = 0): ValueOrPromise { + try { + return fn(); + } catch (e) { + if (isPromise(e) && retryCount < MAX_RETRY_ON_PROMISE_COUNT) { + return e.then(retryOnPromise.bind(null, fn, retryCount++)) as ValueOrPromise; + } + throw e; + } +} diff --git a/packages/qwik/src/core/signal/signal.ts b/packages/qwik/src/core/signal/signal.ts index def7ce37078..f5f80122947 100644 --- a/packages/qwik/src/core/signal/signal.ts +++ b/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 | WrappedSignal).$invalid$ = true; - const previousSignal = signal; - try { - signal = effect; - effect.$effects$?.forEach(scheduleEffect); - } catch (e: unknown) { - logError(e); - } finally { - signal = previousSignal; + if (effect instanceof ComputedSignal || effect instanceof WrappedSignal) { + try { + retryOnPromise(() => effect.$invalidate$()); + } catch (e) { + logError(e); + } } } else if (property === EffectProperty.COMPONENT) { const host: HostElement = effect as any; @@ -458,7 +455,9 @@ export class ComputedSignal extends Signal { 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 extends Signal 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 diff --git a/packages/qwik/src/core/tests/use-computed.spec.tsx b/packages/qwik/src/core/tests/use-computed.spec.tsx index 72c3117c15f..8ecc49a0935 100644 --- a/packages/qwik/src/core/tests/use-computed.spec.tsx +++ b/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 ( +
+ +
+ ); + }); + + const { document } = await render(, { 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 ( From bb11c5f104c3f2c944e58072f433504d7e9cbc92 Mon Sep 17 00:00:00 2001 From: Varixo Date: Wed, 23 Oct 2024 17:02:47 +0200 Subject: [PATCH 2/3] add change --- .changeset/config.json | 9 +-------- .changeset/rotten-weeks-tickle.md | 5 +++++ 2 files changed, 6 insertions(+), 8 deletions(-) create mode 100644 .changeset/rotten-weeks-tickle.md diff --git a/.changeset/config.json b/.changeset/config.json index 8e25df71ac3..89b1506960a 100644 --- a/.changeset/config.json +++ b/.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 } diff --git a/.changeset/rotten-weeks-tickle.md b/.changeset/rotten-weeks-tickle.md new file mode 100644 index 00000000000..267de3a2bbf --- /dev/null +++ b/.changeset/rotten-weeks-tickle.md @@ -0,0 +1,5 @@ +--- +'@qwik.dev/core': patch +--- + +fix: do not trigger effects if computed value is not changed From 874d0260b648a0cdeed1c3e824eca5337f046a50 Mon Sep 17 00:00:00 2001 From: Varixo Date: Thu, 24 Oct 2024 22:13:40 +0200 Subject: [PATCH 3/3] cast instead of condition --- packages/qwik/src/core/signal/signal.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/qwik/src/core/signal/signal.ts b/packages/qwik/src/core/signal/signal.ts index f5f80122947..9857ce87cad 100644 --- a/packages/qwik/src/core/signal/signal.ts +++ b/packages/qwik/src/core/signal/signal.ts @@ -345,12 +345,12 @@ export const triggerEffects = ( container.$scheduler$(ChoreType.QRL_RESOLVE, null, effect.$computeQrl$); } } - if (effect instanceof ComputedSignal || effect instanceof WrappedSignal) { - try { - retryOnPromise(() => effect.$invalidate$()); - } catch (e) { - logError(e); - } + try { + retryOnPromise(() => + (effect as ComputedSignal | WrappedSignal).$invalidate$() + ); + } catch (e) { + logError(e); } } else if (property === EffectProperty.COMPONENT) { const host: HostElement = effect as any;