fix: Await should fallback on route params navigations (#9181)

* fix: fallback on route params navigations

* add changeset

* update changeset

* bundle bump

Author: brophdawg11

.changeset/wet-readers-mate.md

@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+fix: Await should fallback on route params navigations (#9181)

docs/guides/deferred.md

@@ -194,13 +194,13 @@ It's all trade-offs, and what's neat about the API design is that it's well suit
 
 ### When does the `<Suspense/>` fallback render?
 
-The `<Await />` component will only throw the promise up the `<Suspense>` boundary on the initial render of the `<Await />` component with an unsettled promise. It will not re-render the fallback if props change. Effectively, this means that you will not get a fallback rendered when a user submits a form and loader data is revalidated and you will not get a fallback rendered when the user navigates to the same route with different params (in the context of our above example, if the user selects from a list of packages on the left to find their location on the right).
+The `<Await />` component will only throw the promise up the `<Suspense>` boundary on the initial render of the `<Await />` component with an unsettled promise. It will not re-render the fallback if props change. Effectively, this means that you _will not_ get a fallback rendered when a user submits a form and loader data is revalidated. You _will_ get a fallback rendered when the user navigates to the same route with different params (in the context of our above example, if the user selects from a list of packages on the left to find their location on the right).
 
-This may feel counter-intuitive at first, but stay with us, we really thought this through and it's important that it works this way. Let's imagine a world without the deferred API. For those scenarios you're probably going to want to implement Optimistic UI for form submissions/revalidation and some Pending UI for sibling route navigations.
+This may feel counter-intuitive at first, but stay with us, we really thought this through and it's important that it works this way. Let's imagine a world without the deferred API. For those scenarios you're probably going to want to implement Optimistic UI for form submissions/revalidation.
 
-When you decide you'd like to try the trade-offs of `defer`, we don't want you to have to change or remove those optimizations because we want you to be able to easily switch between deferring some data and not deferring it. So we ensure that your existing pending states work the same way. If we didn't do this, then you could experience what we call "Popcorn UI" where submissions of data trigger the fallback loading state instead of the optimistic UI you'd worked hard on.
+When you decide you'd like to try the trade-offs of `defer`, we don't want you to have to change or remove those optimizations because we want you to be able to easily switch between deferring some data and not deferring it. So we ensure that your existing optimistic states work the same way. If we didn't do this, then you could experience what we call "Popcorn UI" where submissions of data trigger the fallback loading state instead of the optimistic UI you'd worked hard on.
 
-So just keep this in mind: **Deferred is 100% only about the initial load of a route.**
+So just keep this in mind: **Deferred is 100% only about the initial load of a route and it's params.**
 
 [link]: ../components/link
 [usefetcher]: ../hooks/use-fetcher

package.json

@@ -101,7 +101,7 @@
   },
   "filesize": {
     "packages/router/dist/router.js": {
-      "none": "98 kB"
+      "none": "99 kB"
     },
     "packages/react-router/dist/react-router.production.min.js": {
       "none": "12 kB"

packages/router/__tests__/router-test.ts

@@ -8635,6 +8635,118 @@ describe("a router", () => {
       expect(shouldRevalidateSpy).not.toHaveBeenCalled();
     });
 
+    it("triggers fallbacks on new dynamic route instances", async () => {
+      let t = setup({
+        routes: [
+          {
+            id: "index",
+            index: true,
+            loader: true,
+          },
+          {
+            id: "invoice",
+            path: "invoices/:id",
+            loader: true,
+          },
+        ],
+        hydrationData: { loaderData: { index: "INDEX" } },
+        initialEntries: ["/"],
+      });
+
+      let A = await t.navigate("/invoices/1");
+      let dfd1 = createDeferred();
+      await A.loaders.invoice.resolve(defer({ lazy: dfd1.promise }));
+      expect(t.router.state.loaderData).toEqual({
+        invoice: {
+          lazy: expect.trackedPromise(),
+        },
+      });
+
+      await dfd1.resolve("DATA 1");
+      expect(t.router.state.loaderData).toEqual({
+        invoice: {
+          lazy: expect.trackedPromise("DATA 1"),
+        },
+      });
+
+      // Goes back into a loading state since this is a new instance of the
+      // invoice route
+      let B = await t.navigate("/invoices/2");
+      let dfd2 = createDeferred();
+      await B.loaders.invoice.resolve(defer({ lazy: dfd2.promise }));
+      expect(t.router.state.loaderData).toEqual({
+        invoice: {
+          lazy: expect.trackedPromise(),
+        },
+      });
+
+      await dfd2.resolve("DATA 2");
+      expect(t.router.state.loaderData).toEqual({
+        invoice: {
+          lazy: expect.trackedPromise("DATA 2"),
+        },
+      });
+    });
+
+    it("triggers fallbacks on new splat route instances", async () => {
+      let t = setup({
+        routes: [
+          {
+            id: "index",
+            index: true,
+            loader: true,
+          },
+          {
+            id: "invoices",
+            path: "invoices",
+            children: [
+              {
+                id: "invoice",
+                path: "*",
+                loader: true,
+              },
+            ],
+          },
+        ],
+        hydrationData: { loaderData: { index: "INDEX" } },
+        initialEntries: ["/"],
+      });
+
+      let A = await t.navigate("/invoices/1");
+      let dfd1 = createDeferred();
+      await A.loaders.invoice.resolve(defer({ lazy: dfd1.promise }));
+      expect(t.router.state.loaderData).toEqual({
+        invoice: {
+          lazy: expect.trackedPromise(),
+        },
+      });
+
+      await dfd1.resolve("DATA 1");
+      expect(t.router.state.loaderData).toEqual({
+        invoice: {
+          lazy: expect.trackedPromise("DATA 1"),
+        },
+      });
+
+      // Goes back into a loading state since this is a new instance of the
+      // invoice route
+      let B = await t.navigate("/invoices/2");
+      let dfd2 = createDeferred();
+      await B.loaders.invoice.resolve(defer({ lazy: dfd2.promise }));
+      expect(t.router.state.loaderData).toEqual({
+        invoice: {
+          lazy: expect.trackedPromise(),
+        },
+      });
+
+      await dfd2.resolve("DATA 2");
+      expect(t.router.state.loaderData).toEqual({
+        invoice: {
+          lazy: expect.trackedPromise("DATA 2"),
+        },
+      });
+    });
+
     it("cancels awaited reused deferreds on subsequent navigations", async () => {
       let shouldRevalidateSpy = jest.fn(() => false);
       let t = setup({

packages/router/router.ts

@@ -1025,6 +1025,7 @@ export function createRouter(init: RouterInit): Router {
 
     let { results, loaderResults, fetcherResults } =
       await callLoadersAndMaybeResolveData(
+        state.matches,
         matchesToLoad,
         revalidatingFetchers,
         request
@@ -1256,6 +1257,7 @@ export function createRouter(init: RouterInit): Router {
 
     let { results, loaderResults, fetcherResults } =
       await callLoadersAndMaybeResolveData(
+        state.matches,
         matchesToLoad,
         revalidatingFetchers,
         revalidationRequest
@@ -1461,6 +1463,7 @@ export function createRouter(init: RouterInit): Router {
   }
 
   async function callLoadersAndMaybeResolveData(
+    currentMatches: AgnosticDataRouteMatch[],
     matchesToLoad: AgnosticDataRouteMatch[],
     fetchersToLoad: RevalidatingFetcher[],
     request: Request
@@ -1479,13 +1482,15 @@ export function createRouter(init: RouterInit): Router {
 
     await Promise.all([
       resolveDeferredResults(
+        currentMatches,
         matchesToLoad,
         loaderResults,
         request.signal,
         false,
         state.loaderData
       ),
       resolveDeferredResults(
+        currentMatches,
         fetchersToLoad.map(([, , match]) => match),
         fetcherResults,
         request.signal,
@@ -2197,6 +2202,20 @@ function isNewLoader(
   return isNew || isMissingData;
 }
 
+function isNewRouteInstance(
+  currentMatch: AgnosticDataRouteMatch,
+  match: AgnosticDataRouteMatch
+) {
+  return (
+    // param change for this match, /users/123 -> /users/456
+    currentMatch.pathname !== match.pathname ||
+    // splat param changed, which is not present in match.path
+    // e.g. /files/images/avatar.jpg -> files/finances.xls
+    (currentMatch.route.path?.endsWith("*") &&
+      currentMatch.params["*"] !== match.params["*"])
+  );
+}
+
 function shouldRevalidateLoader(
   currentLocation: string | Location,
   currentMatch: AgnosticDataRouteMatch,
@@ -2218,12 +2237,7 @@ function shouldRevalidateLoader(
   // Note that fetchers always provide the same current/next locations so the
   // URL-based checks here don't apply to fetcher shouldRevalidate calls
   let defaultShouldRevalidate =
-    // param change for this match, /users/123 -> /users/456
-    currentMatch.pathname !== match.pathname ||
-    // splat param changed, which is not present in match.path
-    // e.g. /files/images/avatar.jpg -> files/finances.xls
-    (currentMatch.route.path?.endsWith("*") &&
-      currentMatch.params["*"] !== match.params["*"]) ||
+    isNewRouteInstance(currentMatch, match) ||
     // Clicked the same link, resubmitted a GET form
     currentUrl.toString() === nextUrl.toString() ||
     // Search params affect all loaders
@@ -2629,6 +2643,7 @@ function isRedirectResult(result?: DataResult): result is RedirectResult {
 }
 
 async function resolveDeferredResults(
+  currentMatches: AgnosticDataRouteMatch[],
   matchesToLoad: AgnosticDataRouteMatch[],
   results: DataResult[],
   signal: AbortSignal,
@@ -2637,11 +2652,16 @@ async function resolveDeferredResults(
 ) {
   for (let index = 0; index < results.length; index++) {
     let result = results[index];
-    let id = matchesToLoad[index].route.id;
-    if (
-      isDeferredResult(result) &&
-      (isFetcher || currentLoaderData?.[id] !== undefined)
-    ) {
+    let match = matchesToLoad[index];
+    let currentMatch = currentMatches.find(
+      (m) => m.route.id === match.route.id
+    );
+    let isRevalidatingLoader =
+      currentMatch != null &&
+      !isNewRouteInstance(currentMatch, match) &&
+      currentLoaderData?.[match.route.id] !== undefined;
+
+    if (isDeferredResult(result) && (isFetcher || isRevalidatingLoader)) {
       // Note: we do not have to touch activeDeferreds here since we race them
       // against the signal in resolveDeferredData and they'll get aborted
       // there if needed