From a6dde01d96654b72f5a5a047cf3422210e260834 Mon Sep 17 00:00:00 2001 From: Sheraff Date: Sat, 16 Aug 2025 10:55:53 +0200 Subject: [PATCH 1/4] refactor(router-core): Verify changes are necessary before updating store --- .../store-updates-during-navigation.test.tsx | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/packages/react-router/tests/store-updates-during-navigation.test.tsx b/packages/react-router/tests/store-updates-during-navigation.test.tsx index 9a81d91f13..8768f65895 100644 --- a/packages/react-router/tests/store-updates-during-navigation.test.tsx +++ b/packages/react-router/tests/store-updates-during-navigation.test.tsx @@ -104,12 +104,15 @@ async function run({ select }: ReturnType) { return after - before } +function resolveAfter(ms: number, value: any) { + return new Promise((resolve) => setTimeout(() => resolve(value), ms)) +} + describe("Store doesn't update *too many* times during navigation", () => { test('async loader, async beforeLoad, pendingMs', async () => { const params = setup({ - beforeLoad: () => - new Promise((resolve) => setTimeout(resolve, 100)), - loader: () => new Promise((resolve) => setTimeout(resolve, 100)), + beforeLoad: () => resolveAfter(100, { foo: 'bar' }), + loader: () => resolveAfter(100, { hello: 'world' }), defaultPendingMs: 100, defaultPendingMinMs: 300, }) @@ -119,7 +122,7 @@ describe("Store doesn't update *too many* times during navigation", () => { // This number should be as small as possible to minimize the amount of work // that needs to be done during a navigation. // Any change that increases this number should be investigated. - expect(updates).toBe(14) + expect(updates).toBe(10) }) test('redirection in preload', async () => { @@ -137,13 +140,13 @@ describe("Store doesn't update *too many* times during navigation", () => { // This number should be as small as possible to minimize the amount of work // that needs to be done during a navigation. // Any change that increases this number should be investigated. - expect(updates).toBe(6) + expect(updates).toBe(5) }) test('sync beforeLoad', async () => { const params = setup({ beforeLoad: () => ({ foo: 'bar' }), - loader: () => new Promise((resolve) => setTimeout(resolve, 100)), + loader: () => resolveAfter(100, { hello: 'world' }), defaultPendingMs: 100, defaultPendingMinMs: 300, }) @@ -153,7 +156,7 @@ describe("Store doesn't update *too many* times during navigation", () => { // This number should be as small as possible to minimize the amount of work // that needs to be done during a navigation. // Any change that increases this number should be investigated. - expect(updates).toBe(13) + expect(updates).toBe(9) }) test('nothing', async () => { @@ -164,8 +167,8 @@ describe("Store doesn't update *too many* times during navigation", () => { // This number should be as small as possible to minimize the amount of work // that needs to be done during a navigation. // Any change that increases this number should be investigated. - expect(updates).toBeGreaterThanOrEqual(10) // WARN: this is flaky, and sometimes (rarely) is 11 - expect(updates).toBeLessThanOrEqual(11) + expect(updates).toBeGreaterThanOrEqual(6) // WARN: this is flaky, and sometimes (rarely) is 7 + expect(updates).toBeLessThanOrEqual(7) }) test('not found in beforeLoad', async () => { @@ -205,6 +208,6 @@ describe("Store doesn't update *too many* times during navigation", () => { // This number should be as small as possible to minimize the amount of work // that needs to be done during a navigation. // Any change that increases this number should be investigated. - expect(updates).toBe(19) + expect(updates).toBe(15) }) }) From 05b5122fa0c0977ca80da2e0c00bb643188ef709 Mon Sep 17 00:00:00 2001 From: Sheraff Date: Sat, 16 Aug 2025 10:56:58 +0200 Subject: [PATCH 2/4] forgot a file in the commit --- packages/router-core/src/router.ts | 57 +++++++++++++++++------------- 1 file changed, 33 insertions(+), 24 deletions(-) diff --git a/packages/router-core/src/router.ts b/packages/router-core/src/router.ts index 0008552246..86caa71cae 100644 --- a/packages/router-core/src/router.ts +++ b/packages/router-core/src/router.ts @@ -2697,10 +2697,12 @@ export class RouterCore< this.getMatch(matchId), loaderData, ) - innerLoadContext.updateMatch(matchId, (prev) => ({ - ...prev, - loaderData, - })) + if (loaderData !== undefined) { + innerLoadContext.updateMatch(matchId, (prev) => ({ + ...prev, + loaderData, + })) + } } // Lazy option can modify the route options, @@ -2837,14 +2839,18 @@ export class RouterCore< ) : shouldReloadOption - innerLoadContext.updateMatch(matchId, (prev) => { - prev._nonReactive.loaderPromise = createControlledPromise() - return { - ...prev, - preload: - !!preload && !this.state.matches.some((d) => d.id === matchId), - } - }) + const nextPreload = + !!preload && !this.state.matches.some((d) => d.id === matchId) + const match = this.getMatch(matchId)! + match._nonReactive.loaderPromise = createControlledPromise() + if (nextPreload !== match.preload) { + innerLoadContext.updateMatch(matchId, (prev) => { + return { + ...prev, + preload: nextPreload, + } + }) + } // If the route is successful and still fresh, just resolve const { status, invalid } = this.getMatch(matchId)! @@ -2886,23 +2892,26 @@ export class RouterCore< } } } + const match = this.getMatch(matchId)! if (!loaderIsRunningAsync) { - const match = this.getMatch(matchId)! match._nonReactive.loaderPromise?.resolve() match._nonReactive.loadPromise?.resolve() } - innerLoadContext.updateMatch(matchId, (prev) => { - clearTimeout(prev._nonReactive.pendingTimeout) - prev._nonReactive.pendingTimeout = undefined - if (!loaderIsRunningAsync) prev._nonReactive.loaderPromise = undefined - prev._nonReactive.dehydrated = undefined - return { - ...prev, - isFetching: loaderIsRunningAsync ? prev.isFetching : false, - invalid: false, - } - }) + clearTimeout(match._nonReactive.pendingTimeout) + match._nonReactive.pendingTimeout = undefined + if (!loaderIsRunningAsync) match._nonReactive.loaderPromise = undefined + match._nonReactive.dehydrated = undefined + const nextIsFetching = loaderIsRunningAsync ? match.isFetching : false + if (nextIsFetching !== match.isFetching || match.invalid !== false) { + innerLoadContext.updateMatch(matchId, (prev) => { + return { + ...prev, + isFetching: nextIsFetching, + invalid: false, + } + }) + } return this.getMatch(matchId)! } From db78d61c8dba1fbc331b89de1ffd1919f9055563 Mon Sep 17 00:00:00 2001 From: Sheraff Date: Sat, 16 Aug 2025 11:02:46 +0200 Subject: [PATCH 3/4] cleanup --- packages/router-core/src/router.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/router-core/src/router.ts b/packages/router-core/src/router.ts index 86caa71cae..9ed6fdb667 100644 --- a/packages/router-core/src/router.ts +++ b/packages/router-core/src/router.ts @@ -2904,13 +2904,11 @@ export class RouterCore< match._nonReactive.dehydrated = undefined const nextIsFetching = loaderIsRunningAsync ? match.isFetching : false if (nextIsFetching !== match.isFetching || match.invalid !== false) { - innerLoadContext.updateMatch(matchId, (prev) => { - return { - ...prev, - isFetching: nextIsFetching, - invalid: false, - } - }) + innerLoadContext.updateMatch(matchId, (prev) => ({ + ...prev, + isFetching: nextIsFetching, + invalid: false, + })) } return this.getMatch(matchId)! } From 4a45c06bf59962200dac2cf61c236b73ea6f36e4 Mon Sep 17 00:00:00 2001 From: Sheraff Date: Sat, 16 Aug 2025 13:32:30 +0200 Subject: [PATCH 4/4] cleanup --- packages/router-core/src/router.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/router-core/src/router.ts b/packages/router-core/src/router.ts index 9ed6fdb667..8321ed22c6 100644 --- a/packages/router-core/src/router.ts +++ b/packages/router-core/src/router.ts @@ -2844,12 +2844,10 @@ export class RouterCore< const match = this.getMatch(matchId)! match._nonReactive.loaderPromise = createControlledPromise() if (nextPreload !== match.preload) { - innerLoadContext.updateMatch(matchId, (prev) => { - return { - ...prev, - preload: nextPreload, - } - }) + innerLoadContext.updateMatch(matchId, (prev) => ({ + ...prev, + preload: nextPreload, + })) } // If the route is successful and still fresh, just resolve