From 89d32096a97a79bb234284aed868c10b322201e8 Mon Sep 17 00:00:00 2001 From: Benny Joo Date: Fri, 20 Oct 2023 14:03:19 +0100 Subject: [PATCH 1/3] fix: first solution using RouterContext --- .../lib/hooks/useParamsWithFallback.test.ts | 49 +++++++++++++++++++ packages/lib/hooks/useParamsWithFallback.ts | 20 ++++++-- setupVitest.ts | 12 +++++ 3 files changed, 77 insertions(+), 4 deletions(-) create mode 100644 packages/lib/hooks/useParamsWithFallback.test.ts diff --git a/packages/lib/hooks/useParamsWithFallback.test.ts b/packages/lib/hooks/useParamsWithFallback.test.ts new file mode 100644 index 00000000000000..c1714bfaa55003 --- /dev/null +++ b/packages/lib/hooks/useParamsWithFallback.test.ts @@ -0,0 +1,49 @@ +import { renderHook } from "@testing-library/react-hooks"; +import { vi } from "vitest"; +import { describe, expect, it } from "vitest"; + +import { useParamsWithFallback } from "./useParamsWithFallback"; + +describe("useParamsWithFallback hook", () => { + it("should return router.query when router.query exists", () => { + vi.mock("next/navigation", () => ({ + useParams: vi.fn().mockReturnValue(null), + })); + + vi.mock("react", () => ({ + useContext: vi.fn().mockReturnValue({ query: { id: 1 } }), + })); + + const { result } = renderHook(() => useParamsWithFallback()); + + expect(result.current).toEqual({ id: 1 }); + }); + + it("should return useParams() when router is undefined", () => { + vi.mock("next/navigation", () => ({ + useParams: vi.fn().mockReturnValue({ id: 1 }), + })); + + vi.mock("react", () => ({ + useContext: vi.fn().mockReturnValue(undefined), + })); + + const { result } = renderHook(() => useParamsWithFallback()); + + expect(result.current).toEqual({ id: 1 }); + }); + + it("should return useParams() when router is null", () => { + vi.mock("next/navigation", () => ({ + useParams: vi.fn().mockReturnValue({ id: 1 }), + })); + + vi.mock("react", () => ({ + useContext: vi.fn().mockReturnValue(null), + })); + + const { result } = renderHook(() => useParamsWithFallback()); + + expect(result.current).toEqual({ id: 1 }); + }); +}); diff --git a/packages/lib/hooks/useParamsWithFallback.ts b/packages/lib/hooks/useParamsWithFallback.ts index 53dd1bb4989db9..f18498b4a30e9e 100644 --- a/packages/lib/hooks/useParamsWithFallback.ts +++ b/packages/lib/hooks/useParamsWithFallback.ts @@ -1,13 +1,25 @@ "use client"; +import { RouterContext } from "next/dist/shared/lib/router-context"; import { useParams } from "next/navigation"; -import { useRouter } from "next/router"; +import { useContext } from "react"; + +interface Params { + [key: string]: string | string[]; +} /** * This hook is a workaround until pages are migrated to app directory. */ -export function useParamsWithFallback() { - const router = useRouter(); +export function useParamsWithFallback(): Params { const params = useParams(); - return params || router.query; + // `Error: NextRouter was not mounted` is thrown if `useRouter` from `next/router` is called in App router. + // As can be seen in https://github.com/vercel/next.js/blob/e8a92a9507cff7d5f7b52701089d4b8141126a63/packages/next/src/client/router.ts#L132, + // `useRouter()` hook returns `React.useContext(RouterContext)`. + // Hence, We can directly use this value. + const router = useContext(RouterContext); + if (router) { + return (router.query ?? {}) as Params; + } + return params ?? {}; } diff --git a/setupVitest.ts b/setupVitest.ts index 18104fa24c0f41..3b671f5b972983 100644 --- a/setupVitest.ts +++ b/setupVitest.ts @@ -1,3 +1,4 @@ +import { JSDOM } from "jsdom"; import { vi } from "vitest"; import createFetchMock from "vitest-fetch-mock"; @@ -5,3 +6,14 @@ const fetchMocker = createFetchMock(vi); // sets globalThis.fetch and globalThis.fetchMock to our mocked version fetchMocker.enableMocks(); + +const jsdom = new JSDOM("", { + url: "http://localhost", +}); + +global.window = jsdom.window; +global.document = jsdom.window.document; +global.navigator = { + ...global.navigator, + userAgent: "node.js", +}; From cf5a29522c684ec266c0cf555f5791e59e8ee02c Mon Sep 17 00:00:00 2001 From: Benny Joo Date: Thu, 2 Nov 2023 18:04:57 +0000 Subject: [PATCH 2/3] fix: second solution by importing router from next/compat/router --- .../lib/hooks/useParamsWithFallback.test.ts | 32 +++++++++++++------ packages/lib/hooks/useParamsWithFallback.ts | 16 +++------- setupVitest.ts | 12 ------- vitest.workspace.ts | 9 +++++- 4 files changed, 35 insertions(+), 34 deletions(-) diff --git a/packages/lib/hooks/useParamsWithFallback.test.ts b/packages/lib/hooks/useParamsWithFallback.test.ts index c1714bfaa55003..0a8ff38dd7c3cf 100644 --- a/packages/lib/hooks/useParamsWithFallback.test.ts +++ b/packages/lib/hooks/useParamsWithFallback.test.ts @@ -5,13 +5,13 @@ import { describe, expect, it } from "vitest"; import { useParamsWithFallback } from "./useParamsWithFallback"; describe("useParamsWithFallback hook", () => { - it("should return router.query when router.query exists", () => { + it("should return router.query when param is null", () => { vi.mock("next/navigation", () => ({ useParams: vi.fn().mockReturnValue(null), })); - vi.mock("react", () => ({ - useContext: vi.fn().mockReturnValue({ query: { id: 1 } }), + vi.mock("next/compat/router", () => ({ + useRouter: vi.fn().mockReturnValue({ query: { id: 1 } }), })); const { result } = renderHook(() => useParamsWithFallback()); @@ -19,13 +19,27 @@ describe("useParamsWithFallback hook", () => { expect(result.current).toEqual({ id: 1 }); }); - it("should return useParams() when router is undefined", () => { + it("should return router.query when param is undefined", () => { + vi.mock("next/navigation", () => ({ + useParams: vi.fn().mockReturnValue(undefined), + })); + + vi.mock("next/compat/router", () => ({ + useRouter: vi.fn().mockReturnValue({ query: { id: 1 } }), + })); + + const { result } = renderHook(() => useParamsWithFallback()); + + expect(result.current).toEqual({ id: 1 }); + }); + + it("should return useParams() if it exists", () => { vi.mock("next/navigation", () => ({ useParams: vi.fn().mockReturnValue({ id: 1 }), })); - vi.mock("react", () => ({ - useContext: vi.fn().mockReturnValue(undefined), + vi.mock("next/compat/router", () => ({ + useRouter: vi.fn().mockReturnValue(null), })); const { result } = renderHook(() => useParamsWithFallback()); @@ -33,13 +47,13 @@ describe("useParamsWithFallback hook", () => { expect(result.current).toEqual({ id: 1 }); }); - it("should return useParams() when router is null", () => { + it("should return useParams() if it exists", () => { vi.mock("next/navigation", () => ({ useParams: vi.fn().mockReturnValue({ id: 1 }), })); - vi.mock("react", () => ({ - useContext: vi.fn().mockReturnValue(null), + vi.mock("next/compat/router", () => ({ + useRouter: vi.fn().mockReturnValue({ query: { id: 2 } }), })); const { result } = renderHook(() => useParamsWithFallback()); diff --git a/packages/lib/hooks/useParamsWithFallback.ts b/packages/lib/hooks/useParamsWithFallback.ts index f18498b4a30e9e..57d627d44ddcf9 100644 --- a/packages/lib/hooks/useParamsWithFallback.ts +++ b/packages/lib/hooks/useParamsWithFallback.ts @@ -1,8 +1,7 @@ "use client"; -import { RouterContext } from "next/dist/shared/lib/router-context"; +import { useRouter as useCompatRouter } from "next/compat/router"; import { useParams } from "next/navigation"; -import { useContext } from "react"; interface Params { [key: string]: string | string[]; @@ -12,14 +11,7 @@ interface Params { * This hook is a workaround until pages are migrated to app directory. */ export function useParamsWithFallback(): Params { - const params = useParams(); - // `Error: NextRouter was not mounted` is thrown if `useRouter` from `next/router` is called in App router. - // As can be seen in https://github.com/vercel/next.js/blob/e8a92a9507cff7d5f7b52701089d4b8141126a63/packages/next/src/client/router.ts#L132, - // `useRouter()` hook returns `React.useContext(RouterContext)`. - // Hence, We can directly use this value. - const router = useContext(RouterContext); - if (router) { - return (router.query ?? {}) as Params; - } - return params ?? {}; + const params = useParams(); // always `null` in pages router + const router = useCompatRouter(); // always `null` in app router + return params ?? router?.query ?? {}; } diff --git a/setupVitest.ts b/setupVitest.ts index 3b671f5b972983..18104fa24c0f41 100644 --- a/setupVitest.ts +++ b/setupVitest.ts @@ -1,4 +1,3 @@ -import { JSDOM } from "jsdom"; import { vi } from "vitest"; import createFetchMock from "vitest-fetch-mock"; @@ -6,14 +5,3 @@ const fetchMocker = createFetchMock(vi); // sets globalThis.fetch and globalThis.fetchMock to our mocked version fetchMocker.enableMocks(); - -const jsdom = new JSDOM("", { - url: "http://localhost", -}); - -global.window = jsdom.window; -global.document = jsdom.window.document; -global.navigator = { - ...global.navigator, - userAgent: "node.js", -}; diff --git a/vitest.workspace.ts b/vitest.workspace.ts index 20d12799fb9902..1b59d9ba8066ef 100644 --- a/vitest.workspace.ts +++ b/vitest.workspace.ts @@ -36,7 +36,7 @@ const workspaces = packagedEmbedTestsOnly test: { include: ["packages/**/*.{test,spec}.{ts,js}", "apps/**/*.{test,spec}.{ts,js}"], // TODO: Ignore the api until tests are fixed - exclude: ["**/node_modules/**/*", "packages/embeds/**/*"], + exclude: ["**/node_modules/**/*", "packages/embeds/**/*", "packages/lib/hooks/**/*"], setupFiles: ["setupVitest.ts"], }, }, @@ -67,6 +67,13 @@ const workspaces = packagedEmbedTestsOnly setupFiles: ["packages/app-store/test-setup.ts"], }, }, + { + test: { + name: "@calcom/packages/lib/hooks", + include: ["packages/lib/hooks/**/*.{test,spec}.{ts,js}"], + environment: "jsdom", + }, + }, ]; export default defineWorkspace(workspaces); From 08b84cd58a3f10283f2bb5a0132d7407d12eca88 Mon Sep 17 00:00:00 2001 From: Benny Joo Date: Fri, 3 Nov 2023 16:06:07 +0000 Subject: [PATCH 3/3] fix return type --- packages/lib/hooks/useParamsWithFallback.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/lib/hooks/useParamsWithFallback.ts b/packages/lib/hooks/useParamsWithFallback.ts index 57d627d44ddcf9..8e5e4eb5f38115 100644 --- a/packages/lib/hooks/useParamsWithFallback.ts +++ b/packages/lib/hooks/useParamsWithFallback.ts @@ -2,6 +2,7 @@ import { useRouter as useCompatRouter } from "next/compat/router"; import { useParams } from "next/navigation"; +import type { ParsedUrlQuery } from "querystring"; interface Params { [key: string]: string | string[]; @@ -10,7 +11,7 @@ interface Params { /** * This hook is a workaround until pages are migrated to app directory. */ -export function useParamsWithFallback(): Params { +export function useParamsWithFallback(): Params | ParsedUrlQuery { const params = useParams(); // always `null` in pages router const router = useCompatRouter(); // always `null` in app router return params ?? router?.query ?? {};