chore: upgrade jest and jsdom

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 6, 2023 · e516138 · e516138
1 parent 4ccdc11
commit e516138
Show file tree

Hide file tree

Showing 11 changed files with 894 additions and 766 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
@@ -302,6 +302,7 @@ type SetupOpts = {
   future?: FutureConfig;
 };
 
+let globalWindow = window;
 function setup({
   routes,
   basename,
@@ -428,6 +429,15 @@ function setup({
     });
   }
 
+  // jsdom is making more and more properties non-configurable, so we inject our own jest-friendly window 😅
+  let window = {
+    ...globalWindow,
+    location: {
+      ...globalWindow.location,
+      assign: jest.fn(),
+      replace: jest.fn(),
+    },
+  } as unknown as Window; // spread makes TS sad, since `window.NaN` conflicts with the `[index: number]: Window` index signature
   let history = createMemoryHistory({ initialEntries, initialIndex });
   jest.spyOn(history, "push");
   jest.spyOn(history, "replace");
@@ -437,6 +447,7 @@ function setup({
     routes: enhanceRoutes(routes),
     hydrationData,
     future,
+    window,
   }).initialize();
 
   function getRouteHelpers(
@@ -843,6 +854,7 @@ function setup({
   }
 
   return {
+    window,
     history,
     router: currentRouter,
     navigate,
@@ -6496,15 +6508,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", {
@@ -6513,10 +6516,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();
       }
     });
 
@@ -6529,15 +6530,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", {
@@ -6547,10 +6539,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();
       }
     });
 
@@ -6605,15 +6595,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", {
@@ -6623,10 +6604,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,11 +662,7 @@ 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 globalWindow = typeof window !== "undefined" ? window : undefined;
 
 const defaultMapRouteProperties: MapRoutePropertiesFunction = (route) => ({
   hasErrorBoundary: Boolean(route.hasErrorBoundary),
@@ -680,7 +677,16 @@ const defaultMapRouteProperties: MapRoutePropertiesFunction = (route) => ({
 /**
  * Create a router and listen to history POP navigations
  */
-export function createRouter(init: RouterInit): Router {
+export function createRouter({
+  window = globalWindow,
+  ...init
+}: RouterInit): Router {
+  const isBrowser =
+    typeof window !== "undefined" &&
+    typeof window.document !== "undefined" &&
+    typeof window.document.createElement !== "undefined";
+  const isServer = !isBrowser;
+
   invariant(
     init.routes.length > 0,
     "You must provide a non-empty routes array to createRouter"