chore: upgrade jest and jsdom (#10453)

This lets us remove a polyfill and some other hacks, and brings along some additional jsdom features which will facilitate testing fixes to submitter serialization. Note: * jsdom is making it harder to stub things, so we inject a jest-friendly window into the router. I'm not a huge fan of DI when used solely for the sake of testing, but this seems less terrible than other approaches (e.g. (patch-package-ing jsdom, or monkey-patching Object.defineProperty) 🙃 * We use a yarn resolution to get the latest jsdom, despite the latest jest pinning it at ^20.0.0. Per the maintainers, this is fine as jest doesn't need any code changes to work with new jsdom, and there are no concrete plans yet to ship a major version.
remix-run · May 25, 2023 · e2789c1 · e2789c1
1 parent 3e6628c
commit e2789c1
Show file tree

Hide file tree

Showing 11 changed files with 900 additions and 774 deletions.
diff --git a/.changeset/stale-birds-travel.md b/.changeset/stale-birds-travel.md
@@ -0,0 +1,5 @@
+---
+"@remix-run/router": patch
+---
+
+upgrade jest and jsdom
diff --git a/package.json b/package.json
@@ -32,11 +32,15 @@
   "jest": {
     "projects": [
       "<rootDir>/packages/*"
+    ],
+    "reporters": [
+      "default"
     ]
   },
   "resolutions": {
     "@types/react": "^18.0.0",
-    "@types/react-dom": "^18.0.0"
+    "@types/react-dom": "^18.0.0",
+    "jsdom": "22.0.0"
   },
   "dependencies": {
     "@ampproject/filesize": "^4.3.0",

diff --git a/packages/react-router-dom-v5-compat/jest.config.js b/packages/react-router-dom-v5-compat/jest.config.js
@@ -21,5 +21,4 @@ module.exports = {
     "^react-router$": "<rootDir>/../react-router/index.ts",
     "^react-router-dom-v5-compat$": "<rootDir>/index.ts",
   },
-  reporters: ["default"],
 };
diff --git a/packages/react-router-dom/__tests__/polyfills/SubmitEvent.submitter.ts b/packages/react-router-dom/__tests__/polyfills/SubmitEvent.submitter.ts
diff --git a/packages/react-router-dom/__tests__/setup.ts b/packages/react-router-dom/__tests__/setup.ts
@@ -5,8 +5,6 @@ import {
 import { fetch, Request, Response, Headers } from "@remix-run/web-fetch";
 import { AbortController as NodeAbortController } from "abort-controller";
 
-import "./polyfills/SubmitEvent.submitter";
-
 // https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html#configuring-your-testing-environment
 globalThis.IS_REACT_ACT_ENVIRONMENT = true;
 

diff --git a/packages/react-router-dom/jest.config.js b/packages/react-router-dom/jest.config.js
@@ -21,5 +21,4 @@ module.exports = {
     "^react-router$": "<rootDir>/../react-router/index.ts",
     "^react-router-dom$": "<rootDir>/index.tsx",
   },
-  reporters: ["default"],
 };
diff --git a/packages/react-router-native/jest.config.js b/packages/react-router-native/jest.config.js
@@ -24,5 +24,4 @@ module.exports = {
     "^react-router$": "<rootDir>/../react-router/index.ts",
     "^react-router-native$": "<rootDir>/index.tsx",
   },
-  reporters: ["default"],
 };
diff --git a/packages/router/__tests__/router-test.ts b/packages/router/__tests__/router-test.ts
@@ -428,6 +428,18 @@ function setup({
     });
   }
 
+  // jsdom is making more and more properties non-configurable, so we inject
+  // our own jest-friendly window.
+  let testWindow = {
+    ...window,
+    location: {
+      ...window.location,
+      assign: jest.fn(),
+      replace: jest.fn(),
+    },
+  } as unknown as Window;
+  // ^ Spread makes TS sad - `window.NaN` conflicts with `[index: number]: Window`
+
   let history = createMemoryHistory({ initialEntries, initialIndex });
   jest.spyOn(history, "push");
   jest.spyOn(history, "replace");
@@ -437,6 +449,7 @@ function setup({
     routes: enhanceRoutes(routes),
     hydrationData,
     future,
+    window: testWindow,
   }).initialize();
 
   function getRouteHelpers(
@@ -843,6 +856,7 @@ function setup({
   }
 
   return {
+    window: testWindow,
     history,
     router: currentRouter,
     navigate,
@@ -6545,15 +6559,6 @@ describe("a router", () => {
       ];
 
       for (let url of urls) {
-        // This is gross, don't blame me, blame SO :)
-        // https://stackoverflow.com/a/60697570
-        let oldLocation = window.location;
-        const location = new URL(window.location.href) as unknown as Location;
-        location.assign = jest.fn();
-        location.replace = jest.fn();
-        delete (window as any).location;
-        window.location = location as unknown as Location;
-
         let t = setup({ routes: REDIRECT_ROUTES });
 
         let A = await t.navigate("/parent/child", {
@@ -6562,10 +6567,8 @@ describe("a router", () => {
         });
 
         await A.actions.child.redirectReturn(url);
-        expect(window.location.assign).toHaveBeenCalledWith(url);
-        expect(window.location.replace).not.toHaveBeenCalled();
-
-        window.location = oldLocation;
+        expect(t.window.location.assign).toHaveBeenCalledWith(url);
+        expect(t.window.location.replace).not.toHaveBeenCalled();
       }
     });
 
@@ -6578,15 +6581,6 @@ describe("a router", () => {
       ];
 
       for (let url of urls) {
-        // This is gross, don't blame me, blame SO :)
-        // https://stackoverflow.com/a/60697570
-        let oldLocation = window.location;
-        const location = new URL(window.location.href) as unknown as Location;
-        location.assign = jest.fn();
-        location.replace = jest.fn();
-        delete (window as any).location;
-        window.location = location as unknown as Location;
-
         let t = setup({ routes: REDIRECT_ROUTES });
 
         let A = await t.navigate("/parent/child", {
@@ -6596,10 +6590,8 @@ describe("a router", () => {
         });
 
         await A.actions.child.redirectReturn(url);
-        expect(window.location.replace).toHaveBeenCalledWith(url);
-        expect(window.location.assign).not.toHaveBeenCalled();
-
-        window.location = oldLocation;
+        expect(t.window.location.replace).toHaveBeenCalledWith(url);
+        expect(t.window.location.assign).not.toHaveBeenCalled();
       }
     });
 
@@ -6654,15 +6646,6 @@ describe("a router", () => {
     });
 
     it("treats same-origin absolute URLs as external if they don't match the basename", async () => {
-      // This is gross, don't blame me, blame SO :)
-      // https://stackoverflow.com/a/60697570
-      let oldLocation = window.location;
-      const location = new URL(window.location.href) as unknown as Location;
-      location.assign = jest.fn();
-      location.replace = jest.fn();
-      delete (window as any).location;
-      window.location = location as unknown as Location;
-
       let t = setup({ routes: REDIRECT_ROUTES, basename: "/base" });
 
       let A = await t.navigate("/base/parent/child", {
@@ -6672,10 +6655,8 @@ describe("a router", () => {
 
       let url = "http://localhost/not/the/same/basename";
       await A.actions.child.redirectReturn(url);
-      expect(window.location.assign).toHaveBeenCalledWith(url);
-      expect(window.location.replace).not.toHaveBeenCalled();
-
-      window.location = oldLocation;
+      expect(t.window.location.assign).toHaveBeenCalledWith(url);
+      expect(t.window.location.replace).not.toHaveBeenCalled();
     });
 
     describe("redirect status code handling", () => {

diff --git a/packages/router/jest.config.js b/packages/router/jest.config.js
@@ -16,5 +16,4 @@ module.exports = {
       "@web3-storage/multipart-parser"
     ),
   },
-  reporters: ["default"],
 };
diff --git a/packages/router/router.ts b/packages/router/router.ts
@@ -352,6 +352,7 @@ export interface RouterInit {
   mapRouteProperties?: MapRoutePropertiesFunction;
   future?: Partial<FutureConfig>;
   hydrationData?: HydrationState;
+  window?: Window;
 }
 
 /**
@@ -661,12 +662,6 @@ export const IDLE_BLOCKER: BlockerUnblocked = {
 
 const ABSOLUTE_URL_REGEX = /^(?:[a-z][a-z0-9+.-]*:|\/\/)/i;
 
-const isBrowser =
-  typeof window !== "undefined" &&
-  typeof window.document !== "undefined" &&
-  typeof window.document.createElement !== "undefined";
-const isServer = !isBrowser;
-
 const defaultMapRouteProperties: MapRoutePropertiesFunction = (route) => ({
   hasErrorBoundary: Boolean(route.hasErrorBoundary),
 });
@@ -681,6 +676,17 @@ const defaultMapRouteProperties: MapRoutePropertiesFunction = (route) => ({
  * Create a router and listen to history POP navigations
  */
 export function createRouter(init: RouterInit): Router {
+  const routerWindow = init.window
+    ? init.window
+    : typeof window !== "undefined"
+    ? window
+    : undefined;
+  const isBrowser =
+    typeof routerWindow !== "undefined" &&
+    typeof routerWindow.document !== "undefined" &&
+    typeof routerWindow.document.createElement !== "undefined";
+  const isServer = !isBrowser;
+
   invariant(
     init.routes.length > 0,
     "You must provide a non-empty routes array to createRouter"
@@ -2087,19 +2093,15 @@ export function createRouter(init: RouterInit): Router {
       "Expected a location on the redirect navigation"
     );
     // Check if this an absolute external redirect that goes to a new origin
-    if (
-      ABSOLUTE_URL_REGEX.test(redirect.location) &&
-      isBrowser &&
-      typeof window?.location !== "undefined"
-    ) {
+    if (ABSOLUTE_URL_REGEX.test(redirect.location) && isBrowser) {
       let url = init.history.createURL(redirect.location);
       let isDifferentBasename = stripBasename(url.pathname, basename) == null;
 
-      if (window.location.origin !== url.origin || isDifferentBasename) {
+      if (routerWindow.location.origin !== url.origin || isDifferentBasename) {
         if (replace) {
-          window.location.replace(redirect.location);
+          routerWindow.location.replace(redirect.location);
         } else {
-          window.location.assign(redirect.location);
+          routerWindow.location.assign(redirect.location);
         }
         return;
       }