Repository: remix-run/remix
Pull request #3721 (2022-07-13 14:44:22)

fix(remix-react): fix `shouldReload` bug for deferred cancellations on fetcher submissions
Closes: #3712

- [x] Tests

Testing Strategy: Unit test + confirmed this fixes the issue in the reproduction provided in the issue


changeset-bot[bot]: ###  ⚠️  No Changeset found

Latest commit: 2d67abe44d380402ac27d97a4ddd2c745984a5fa

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. **If these changes should result in a version bump, you need to add a changeset.**

<details><summary>This PR includes no changesets</summary>

  When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

</details>

[Click here to learn what changesets are, and how to add one](https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/remix-run/remix/new/brophdawg11/deferred-fetcher-shouldReload?filename=.changeset/tame-days-rule.md&value=---%0A%22remix%22%3A%20patch%0A%22%40remix-run%2Freact%22%3A%20patch%0A---%0A%0Afix%3A%20Fix%20shouldReload%20bug%20for%20deferred%20cancellatons%20on%20fetcher%20submissions%0A)



Patch:
diff --git a/packages/remix-react/transition.ts b/packages/remix-react/transition.ts
index 5975b93e8..3d16b1089 100644
--- a/packages/remix-react/transition.ts
+++ b/packages/remix-react/transition.ts
@@ -626,17 +626,21 @@ export function createTransitionManager(init: TransitionManagerInit) {
       if (updates.transition === IDLE_TRANSITION) {
         pendingNavigationController = undefined;
 
-        // Preserve deferred data for routes that were not cancelled, or were
-        // cancelled and returned a subsequent deferred() in actionReload
+        // Preserve deferred data for active routes that were not cancelled, or
+        // were cancelled and are again pending (i.e., returned a subsequent
+        // deferred() in actionReload)
         overrides.deferredLoaderData = [
           ...Object.entries(state.deferredLoaderData),
           ...Object.entries(updates.deferredLoaderData || {}),
         ]
-          .filter(
-            ([routeId]) =>
-              !cancelledDeferredRouteIds.has(routeId) ||
-              pendingNavigationDeferredControllers.has(routeId)
-          )
+          .filter(([routeId]) => {
+            let isActive = (updates.matches || state.matches).some(
+              (m) => m.route.id === routeId
+            );
+            let wasCancelled = cancelledDeferredRouteIds.has(routeId);
+            let isPending = pendingNavigationDeferredControllers.has(routeId);
+            return isActive && (!wasCancelled || isPending);
+          })
           .reduce<Record<string, any>>(
             (acc, [routeId, data]) => Object.assign(acc, { [routeId]: data }),
             {}
@@ -2077,11 +2081,17 @@ async function makeDeferredLoaderData(
               },
             };
 
-            if (signal!.aborted) return;
             update({
               deferredLoaderData: newDeferredLoaderData,
               loaderData: newLoaderData,
             });
+
+            if (
+              Object.keys(newDeferredLoaderData[match.route.id]).length === 0
+            ) {
+              // Mark this deferred as no longer pending once we've updated state
+              pendingNavigationDeferredControllers.delete(match.route.id);
+            }
           });
         }
       }
@@ -2100,9 +2110,7 @@ function abortPendingDeferredControllers(
     let isRouteMatched = matches.some((m) => m.route.id === routeId);
     // Or if this route is about to be reloaded
     let isRouteLoading = matchesToLoad.some((m) => m.route.id === routeId);
-    // TODO: add boundary handling when we do actions - cancel below boundary
-    let foundBoundaryId = false;
-    if (!isRouteMatched || isRouteLoading || foundBoundaryId) {
+    if (!isRouteMatched || isRouteLoading) {
       controller.abort();
       pendingNavigationDeferredControllers.delete(routeId);
       cancelledDeferredRouteIds.add(routeId);


Test patch:
diff --git a/packages/remix-react/__tests__/transition-test.tsx b/packages/remix-react/__tests__/transition-test.tsx
index d36afbcd6..05dcf7012 100644
--- a/packages/remix-react/__tests__/transition-test.tsx
+++ b/packages/remix-react/__tests__/transition-test.tsx
@@ -2401,7 +2401,7 @@ describe("deferred", () => {
           lazy: rootDfds[count].promise,
         });
       },
-      rootShouldReload: shouldReloadSpy,
+      shouldReloads: { root: shouldReloadSpy },
     });
 
     let A = t.navigate.get("/foo?key=value");
@@ -2607,7 +2607,7 @@ describe("deferred", () => {
           lazy: rootDfds[count].promise,
         });
       },
-      rootShouldReload: shouldReloadSpy,
+      shouldReloads: { root: shouldReloadSpy },
     });
 
     let A = t.navigate.get("/foo?a=1");
@@ -2687,6 +2687,38 @@ describe("deferred", () => {
     // cannot be opted out of
     expect(shouldReloadSpy).toHaveBeenCalledTimes(1);
   });
+
+  it("removes pending deferreds on successful resolution to re-enable shouldReload on fetch submissions", async () => {
+    let shouldReloadSpy = jest.fn(() => false);
+    let t = setup({ url: "/", shouldReloads: { foo: shouldReloadSpy } });
+
+    let A = t.navigate.get("/foo");
+    let dfdA = defer();
+    await A.loader.resolve(
+      setupDeferred({
+        value: dfdA.promise,
+      })
+    );
+    await dfdA.resolve("A");
+    expect(t.getState().loaderData).toEqual({
+      root: "ROOT",
+      foo: {
+        value: "A",
+      },
+    });
+    expect(shouldReloadSpy).toHaveBeenCalledTimes(0);
+
+    // Navigate to the same URL forcing a revalidation, which foo can opt out of
+    let B = t.fetch.post("/bar");
+    await B.action.resolve("FETCH ACTION");
+    expect(t.getState().loaderData).toEqual({
+      root: "ROOT",
+      foo: {
+        value: "A",
+      },
+    });
+    expect(shouldReloadSpy).toHaveBeenCalledTimes(1);
+  });
 });
 
 // describe("react-router", () => {
@@ -2780,12 +2812,12 @@ let setup = (
   {
     url,
     rootLoaderOverride,
-    rootShouldReload,
+    shouldReloads,
     initialLoaderData,
   }: {
     url: string;
     rootLoaderOverride?: () => any | Promise<any>;
-    rootShouldReload?: ShouldReloadFunction;
+    shouldReloads?: Record<string, ShouldReloadFunction>;
     initialLoaderData?: any;
   } = { url: "/" }
 ) => {
@@ -2858,7 +2890,7 @@ let setup = (
       CatchBoundary: FakeComponent,
       hasLoader: true,
       loader: rootLoader,
-      ...(rootShouldReload ? { shouldReload: rootShouldReload } : {}),
+      ...(shouldReloads?.root ? { shouldReload: shouldReloads?.root } : {}),
       children: [
         {
           path: "/",
@@ -2866,6 +2898,9 @@ let setup = (
           hasLoader: true,
           loader: createLoader(),
           action: createAction(),
+          ...(shouldReloads?.index
+            ? { shouldReload: shouldReloads?.index }
+            : {}),
           element: {} as any,
           module: "",
         },
@@ -2875,6 +2910,7 @@ let setup = (
           hasLoader: true,
           loader: createLoader(),
           action: createAction(),
+          ...(shouldReloads?.foo ? { shouldReload: shouldReloads?.foo } : {}),
           element: {} as any,
           module: "",
         },
@@ -2884,6 +2920,9 @@ let setup = (
           hasLoader: true,
           loader: createLoader(),
           action: createAction(),
+          ...(shouldReloads?.foobar
+            ? { shouldReload: shouldReloads?.foobar }
+            : {}),
           element: {} as any,
           module: "",
         },
@@ -2893,6 +2932,7 @@ let setup = (
           hasLoader: true,
           loader: createLoader(),
           action: createAction(),
+          ...(shouldReloads?.bar ? { shouldReload: shouldReloads?.bar } : {}),
           element: {} as any,
           module: "",
         },
@@ -2902,6 +2942,7 @@ let setup = (
           hasLoader: true,
           loader: createLoader(),
           action: createAction(),
+          ...(shouldReloads?.baz ? { shouldReload: shouldReloads?.baz } : {}),
           element: {} as any,
           module: "",
         },
@@ -2911,6 +2952,9 @@ let setup = (
           hasLoader: true,
           loader: createLoader(),
           action: createAction(),
+          ...(shouldReloads?.param
+            ? { shouldReload: shouldReloads?.param }
+            : {}),
           element: {} as any,
           module: "",
         },