fix: Commit context together with status to avoid race condition (#6407)

Author: schiller-manuel

packages/react-router/tests/store-updates-during-navigation.test.tsx

@@ -136,7 +136,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).toBeGreaterThanOrEqual(11) // WARN: this is flaky, and sometimes (rarely) is 12
+    expect(updates).toBeGreaterThanOrEqual(10) // WARN: this is flaky, and sometimes (rarely) is 12
     expect(updates).toBeLessThanOrEqual(13)
   })
 
@@ -155,7 +155,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(5)
+    expect(updates).toBe(4)
   })
 
   test('sync beforeLoad', async () => {
@@ -171,7 +171,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).toBeGreaterThanOrEqual(10) // WARN: this is flaky
+    expect(updates).toBeGreaterThanOrEqual(9) // WARN: this is flaky
     expect(updates).toBeLessThanOrEqual(12)
   })
 
@@ -183,7 +183,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).toBeGreaterThanOrEqual(8) // WARN: this is flaky, and sometimes (rarely) is 9
+    expect(updates).toBeGreaterThanOrEqual(6) // WARN: this is flaky, and sometimes (rarely) is 9
     expect(updates).toBeLessThanOrEqual(9)
   })
 
@@ -225,7 +225,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(19)
+    expect(updates).toBe(16)
   })
 
   test('navigate, w/ preloaded & async loaders', async () => {
@@ -241,7 +241,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(9)
+    expect(updates).toBe(7)
   })
 
   test('navigate, w/ preloaded & sync loaders', async () => {
@@ -257,7 +257,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(8)
+    expect(updates).toBe(6)
   })
 
   test('navigate, w/ previous navigation & async loader', async () => {
@@ -273,7 +273,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(7)
+    expect(updates).toBe(5)
   })
 
   test('preload a preloaded route w/ async loader', async () => {
@@ -291,6 +291,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(2)
+    expect(updates).toBe(1)
   })
 })

packages/router-core/src/load-matches.ts

@@ -139,6 +139,7 @@ const handleRedirectAndNotFound = (
     inner.updateMatch(match.id, (prev) => ({
       ...prev,
       status,
+      context: buildMatchContext(inner, match.index),
       isFetching: false,
       error: err,
     }))
@@ -693,6 +694,7 @@ const runLoader = async (
       inner.updateMatch(matchId, (prev) => ({
         ...prev,
         error: undefined,
+        context: buildMatchContext(inner, index),
         status: 'success',
         isFetching: false,
         updatedAt: Date.now(),
@@ -705,6 +707,7 @@ const runLoader = async (
           ...prev,
           status: prev.status === 'pending' ? 'success' : prev.status,
           isFetching: false,
+          context: buildMatchContext(inner, index),
         }))
         return
       }
@@ -731,6 +734,7 @@ const runLoader = async (
       inner.updateMatch(matchId, (prev) => ({
         ...prev,
         error,
+        context: buildMatchContext(inner, index),
         status: 'error',
         isFetching: false,
       }))
@@ -754,13 +758,6 @@ const loadRouteMatch = async (
   let loaderIsRunningAsync = false
   const route = inner.router.looseRoutesById[routeId]!
 
-  const commitContext = () => {
-    inner.updateMatch(matchId, (prev) => ({
-      ...prev,
-      context: buildMatchContext(inner, index),
-    }))
-  }
-
   if (shouldSkipLoader(inner, matchId)) {
     if (inner.router.isServer) {
       return inner.router.getMatch(matchId)!
@@ -827,7 +824,6 @@ const loadRouteMatch = async (
         ;(async () => {
           try {
             await runLoader(inner, matchId, index, route)
-            commitContext()
             const match = inner.router.getMatch(matchId)!
             match._nonReactive.loaderPromise?.resolve()
             match._nonReactive.loadPromise?.resolve()
@@ -854,12 +850,6 @@ const loadRouteMatch = async (
   if (!loaderIsRunningAsync) match._nonReactive.loaderPromise = undefined
   match._nonReactive.dehydrated = undefined
 
-  // Commit context now that loader has completed (or was skipped)
-  // For async loaders, this was already done in the async callback
-  if (!loaderIsRunningAsync) {
-    commitContext()
-  }
-
   const nextIsFetching = loaderIsRunningAsync ? match.isFetching : false
   if (nextIsFetching !== match.isFetching || match.invalid !== false) {
     inner.updateMatch(matchId, (prev) => ({

packages/solid-router/tests/store-updates-during-navigation.test.tsx

@@ -136,7 +136,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).toBeGreaterThanOrEqual(11) // WARN: this is flaky, and sometimes (rarely) is 12
+    expect(updates).toBeGreaterThanOrEqual(10) // WARN: this is flaky, and sometimes (rarely) is 12
     expect(updates).toBeLessThanOrEqual(13)
   })
 
@@ -156,7 +156,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
     // Note: Solid has different update counts than React due to different reactivity
-    expect(updates).toBe(7)
+    expect(updates).toBe(6)
   })
 
   test('sync beforeLoad', async () => {
@@ -173,7 +173,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
     // Note: Solid has different update counts than React due to different reactivity
-    expect(updates).toBe(10)
+    expect(updates).toBe(8)
   })
 
   test('nothing', async () => {
@@ -226,7 +226,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(19)
+    expect(updates).toBe(16)
   })
 
   test('navigate, w/ preloaded & async loaders', async () => {
@@ -242,7 +242,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).toBeGreaterThanOrEqual(11) // WARN: this is flaky, and sometimes (rarely) is 12
+    expect(updates).toBeGreaterThanOrEqual(9) // WARN: this is flaky, and sometimes (rarely) is 12
     expect(updates).toBeLessThanOrEqual(13)
   })
 
@@ -260,7 +260,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
     // Note: Solid has one fewer update than React due to different reactivity
-    expect(updates).toBe(9)
+    expect(updates).toBe(7)
   })
 
   test('navigate, w/ previous navigation & async loader', async () => {
@@ -276,7 +276,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(7)
+    expect(updates).toBe(5)
   })
 
   test('preload a preloaded route w/ async loader', async () => {
@@ -294,6 +294,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(3)
+    expect(updates).toBe(2)
   })
 })

packages/vue-router/tests/store-updates-during-navigation.test.tsx

@@ -138,7 +138,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
     // Note: Vue has different update counts than React/Solid due to different reactivity
-    expect(updates).toBe(33)
+    expect(updates).toBe(27)
   })
 
   test('redirection in preload', async () => {
@@ -157,7 +157,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
     // Note: Vue has different update counts than React/Solid due to different reactivity
-    expect(updates).toBe(13)
+    expect(updates).toBe(10)
   })
 
   test('sync beforeLoad', async () => {
@@ -174,7 +174,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
     // Note: Vue has different update counts than React/Solid due to different reactivity
-    expect(updates).toBe(31)
+    expect(updates).toBe(25)
   })
 
   test('nothing', async () => {
@@ -231,7 +231,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
     // Note: Vue has different update counts than React/Solid due to different reactivity
-    expect(updates).toBe(47)
+    expect(updates).toBe(38)
   })
 
   test('navigate, w/ preloaded & async loaders', async () => {
@@ -248,7 +248,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
     // Note: Vue has different update counts than React/Solid due to different reactivity
-    expect(updates).toBe(24)
+    expect(updates).toBe(18)
   })
 
   test('navigate, w/ preloaded & sync loaders', async () => {
@@ -265,7 +265,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
     // Note: Vue has different update counts than React/Solid due to different reactivity
-    expect(updates).toBe(22)
+    expect(updates).toBe(16)
   })
 
   test('navigate, w/ previous navigation & async loader', async () => {
@@ -282,7 +282,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
     // Note: Vue has different update counts than React/Solid due to different reactivity
-    expect(updates).toBe(18)
+    expect(updates).toBe(12)
   })
 
   test('preload a preloaded route w/ async loader', async () => {
@@ -301,6 +301,6 @@ describe("Store doesn't update *too many* times during navigation", () => {
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
     // Note: Vue has different update counts than React/Solid due to different reactivity
-    expect(updates).toBe(6)
+    expect(updates).toBe(3)
   })
 })