Mechanism to sync server prefetch with client API calls (#1798)

* Warn on API calls during initial render not prefetched * Full prefetch for homepage (commented) * Prefetch utility * Check for queries prefetched that are not needed during render and warn * No need to stringify * Replace useQuery overrides with decoupled cache check (wip) * Observer count for unnecessary prefetch warnings * Remove useQuery override * Test prefetch warnings * Remove inadvertent/unnecessary diff * Remove comments * Remove comment * Update frontends/api/src/ssr/usePrefetchWarnings.test.ts Co-authored-by: Chris Chudzicki <[email protected]> * Remove comment as no longer true * Less specific object assertion --------- Co-authored-by: Chris Chudzicki <[email protected]>
mitodl · Nov 13, 2024 · 1a2b98e · 1a2b98e
1 parent 41d284d
commit 1a2b98e
Show file tree

Hide file tree

Showing 13 changed files with 300 additions and 30 deletions.
diff --git a/frontends/api/package.json b/frontends/api/package.json
@@ -10,6 +10,7 @@
     "./v1": "./src/generated/v1/api.ts",
     "./hooks/*": "./src/hooks/*/index.ts",
     "./constants": "./src/common/constants.ts",
+    "./ssr/*": "./src/ssr/*.ts",
     "./test-utils/factories": "./src/test-utils/factories/index.ts",
     "./test-utils": "./src/test-utils/index.ts"
   },

diff --git a/frontends/api/src/hooks/channels/index.ts b/frontends/api/src/hooks/channels/index.ts
@@ -27,6 +27,7 @@ const useChannelDetail = (channelType: string, channelName: string) => {
     ...channels.detailByType(channelType, channelName),
   })
 }
+
 const useChannelCounts = (channelType: string) => {
   return useQuery({
     ...channels.countsByType(channelType),
@@ -54,4 +55,5 @@ export {
   useChannelsList,
   useChannelPartialUpdate,
   useChannelCounts,
+  channels as channelsKeyFactory,
 }
diff --git a/frontends/api/src/hooks/learningResources/index.ts b/frontends/api/src/hooks/learningResources/index.ts
@@ -500,13 +500,6 @@ const useSchoolsList = () => {
   return useQuery(learningResources.schools())
 }
 
-/*
- * Not intended to be imported except for special cases.
- * It's used in the ResourceCarousel to dynamically build a single useQueries hook
- * from config because a React component cannot conditionally call hooks during renders.
- */
-export { default as learningResourcesKeyFactory } from "./keyFactory"
-
 export {
   useLearningResourcesList,
   useFeaturedLearningResourcesList,
@@ -538,4 +531,5 @@ export {
   useListItemMove,
   usePlatformsList,
   useSchoolsList,
+  learningResources as learningResourcesKeyFactory,
 }
diff --git a/frontends/api/src/hooks/newsEvents/index.ts b/frontends/api/src/hooks/newsEvents/index.ts
@@ -15,4 +15,9 @@ const useNewsEventsDetail = (id: number) => {
   return useQuery(newsEvents.detail(id))
 }
 
-export { useNewsEventsList, useNewsEventsDetail, NewsEventsListFeedTypeEnum }
+export {
+  useNewsEventsList,
+  useNewsEventsDetail,
+  NewsEventsListFeedTypeEnum,
+  newsEvents as newsEventsKeyFactory,
+}
diff --git a/frontends/api/src/hooks/testimonials/index.ts b/frontends/api/src/hooks/testimonials/index.ts
@@ -23,4 +23,8 @@ const useTestimonialDetail = (id: number | undefined) => {
   })
 }
 
-export { useTestimonialDetail, useTestimonialList }
+export {
+  useTestimonialDetail,
+  useTestimonialList,
+  testimonials as testimonialsKeyFactory,
+}
diff --git a/frontends/api/src/hooks/widget_lists/index.ts b/frontends/api/src/hooks/widget_lists/index.ts
@@ -3,8 +3,9 @@ import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"
 import { widgetListsApi } from "../../clients"
 import widgetLists from "./keyFactory"
 import { WidgetInstance } from "api/v0"
+
 /**
- * Query is diabled if id is undefined.
+ * Query is disabled if id is undefined.
  */
 const useWidgetList = (id: number | undefined) => {
   return useQuery({

diff --git a/frontends/api/src/ssr/prefetch.ts b/frontends/api/src/ssr/prefetch.ts
@@ -0,0 +1,13 @@
+import { QueryClient, dehydrate } from "@tanstack/react-query"
+import type { Query } from "@tanstack/react-query"
+
+// Utility to avoid repetition in server components
+export const prefetch = async (queries: (Query | unknown)[]) => {
+  const queryClient = new QueryClient()
+
+  await Promise.all(
+    queries.map((query) => queryClient.prefetchQuery(query as Query)),
+  )
+
+  return dehydrate(queryClient)
+}
diff --git a/frontends/api/src/ssr/usePrefetchWarnings.test.ts b/frontends/api/src/ssr/usePrefetchWarnings.test.ts
@@ -0,0 +1,118 @@
+import { renderHook } from "@testing-library/react"
+import { useQuery } from "@tanstack/react-query"
+import { usePrefetchWarnings } from "./usePrefetchWarnings"
+import { setupReactQueryTest } from "../hooks/test-utils"
+import { urls, factories, setMockResponse } from "../test-utils"
+import {
+  learningResourcesKeyFactory,
+  useLearningResourcesDetail,
+} from "../hooks/learningResources"
+
+jest.mock("./usePrefetchWarnings", () => {
+  const originalModule = jest.requireActual("./usePrefetchWarnings")
+  return {
+    ...originalModule,
+    logQueries: jest.fn(),
+  }
+})
+
+describe("SSR prefetch warnings", () => {
+  beforeEach(() => {
+    jest.spyOn(console, "info").mockImplementation(() => {})
+    jest.spyOn(console, "table").mockImplementation(() => {})
+  })
+
+  it("Warns if a query is requested on the client that has not been prefetched", async () => {
+    const { wrapper, queryClient } = setupReactQueryTest()
+
+    const data = factories.learningResources.resource()
+    setMockResponse.get(urls.learningResources.details({ id: 1 }), data)
+
+    renderHook(() => useLearningResourcesDetail(1), { wrapper })
+
+    renderHook(usePrefetchWarnings, {
+      wrapper,
+      initialProps: { queryClient },
+    })
+
+    expect(console.info).toHaveBeenCalledWith(
+      "The following queries were requested in first render but not prefetched.",
+      "If these queries are user-specific, they cannot be prefetched - responses are cached on public CDN.",
+      "Otherwise, consider fetching on the server with prefetch:",
+    )
+    expect(console.table).toHaveBeenCalledWith(
+      [
+        expect.objectContaining({
+          disabled: false,
+          initialStatus: "loading",
+          key: learningResourcesKeyFactory.detail(1).queryKey,
+          observerCount: 1,
+        }),
+      ],
+      ["hash", "initialStatus", "status", "observerCount", "disabled"],
+    )
+  })
+
+  it("Ignores exempted queries requested on the client that have not been prefetched", async () => {
+    const { wrapper, queryClient } = setupReactQueryTest()
+
+    const data = factories.learningResources.resource()
+    setMockResponse.get(urls.learningResources.details({ id: 1 }), data)
+
+    renderHook(() => useLearningResourcesDetail(1), { wrapper })
+
+    renderHook(usePrefetchWarnings, {
+      wrapper,
+      initialProps: {
+        queryClient,
+        exemptions: [learningResourcesKeyFactory.detail(1).queryKey],
+      },
+    })
+
+    expect(console.info).not.toHaveBeenCalled()
+    expect(console.table).not.toHaveBeenCalled()
+  })
+
+  it("Warns for queries prefetched on the server but not requested on the client", async () => {
+    const { wrapper, queryClient } = setupReactQueryTest()
+
+    const data = factories.learningResources.resource()
+    setMockResponse.get(urls.learningResources.details({ id: 1 }), data)
+
+    // Emulate server prefetch
+    const { unmount } = renderHook(
+      () =>
+        useQuery({
+          ...learningResourcesKeyFactory.detail(1),
+          initialData: data,
+        }),
+      { wrapper },
+    )
+
+    // Removes observer
+    unmount()
+
+    renderHook(usePrefetchWarnings, {
+      wrapper,
+      initialProps: { queryClient },
+    })
+
+    expect(console.info).toHaveBeenCalledWith(
+      "The following queries were prefetched on the server but not accessed during initial render.",
+      "If these queries are no longer in use they should removed from prefetch:",
+    )
+    expect(console.table).toHaveBeenCalledWith(
+      [
+        {
+          disabled: false,
+          hash: JSON.stringify(learningResourcesKeyFactory.detail(1).queryKey),
+          initialStatus: "success",
+          key: learningResourcesKeyFactory.detail(1).queryKey,
+          observerCount: 0,
+          status: "success",
+        },
+      ],
+      ["hash", "initialStatus", "status", "observerCount", "disabled"],
+    )
+  })
+})
diff --git a/frontends/api/src/ssr/usePrefetchWarnings.ts b/frontends/api/src/ssr/usePrefetchWarnings.ts
@@ -0,0 +1,96 @@
+import { useEffect } from "react"
+import type { Query, QueryClient, QueryKey } from "@tanstack/react-query"
+
+const logQueries = (...args: [...string[], Query[]]) => {
+  const queries = args.pop() as Query[]
+  console.info(...args)
+  console.table(
+    queries.map((query) => ({
+      key: query.queryKey,
+      hash: query.queryHash,
+      disabled: query.isDisabled(),
+      initialStatus: query.initialState.status,
+      status: query.state.status,
+      observerCount: query.getObserversCount(),
+    })),
+    ["hash", "initialStatus", "status", "observerCount", "disabled"],
+  )
+}
+
+const PREFETCH_EXEMPT_QUERIES = [["userMe"]]
+
+/**
+ * Call this as high as possible in render tree to detect query usage on
+ * first render.
+ */
+export const usePrefetchWarnings = ({
+  queryClient,
+  exemptions = [],
+}: {
+  queryClient: QueryClient
+  /**
+   * A list of query keys that should be exempted.
+   *
+   * NOTE: This uses react-query's hierarchical key matching, so exempting
+   * ["a", { x: 1 }] will exempt
+   *  - ["a", { x: 1 }]
+   *  - ["a", { x: 1, y: 2 }]
+   *  - ["a", { x: 1, y: 2 }, ...any_other_entries]
+   */
+  exemptions?: QueryKey[]
+}) => {
+  /**
+   * NOTE: React renders components top-down, but effects run bottom-up, so
+   * this effect will run after all child effects.
+   */
+  useEffect(
+    () => {
+      if (process.env.NODE_ENV === "production") {
+        return
+      }
+
+      const cache = queryClient.getQueryCache()
+      const queries = cache.getAll()
+
+      const exempted = [...exemptions, ...PREFETCH_EXEMPT_QUERIES].map((key) =>
+        cache.find(key),
+      )
+
+      const potentialPrefetches = queries.filter(
+        (query) =>
+          !exempted.includes(query) &&
+          query.initialState.status !== "success" &&
+          !query.isDisabled(),
+      )
+
+      if (potentialPrefetches.length > 0) {
+        logQueries(
+          "The following queries were requested in first render but not prefetched.",
+          "If these queries are user-specific, they cannot be prefetched - responses are cached on public CDN.",
+          "Otherwise, consider fetching on the server with prefetch:",
+          potentialPrefetches,
+        )
+      }
+
+      const unusedPrefetches = queries.filter(
+        (query) =>
+          !exempted.includes(query) &&
+          query.initialState.status === "success" &&
+          query.getObserversCount() === 0 &&
+          !query.isDisabled(),
+      )
+
+      if (unusedPrefetches.length > 0) {
+        logQueries(
+          "The following queries were prefetched on the server but not accessed during initial render.",
+          "If these queries are no longer in use they should removed from prefetch:",
+          unusedPrefetches,
+        )
+      }
+    },
+    // We only want to run this on initial render.
+    // (Aside: queryClient should be a singleton anyway)
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [],
+  )
+}
diff --git a/frontends/main/src/app-pages/HomePage/HomePage.tsx b/frontends/main/src/app-pages/HomePage/HomePage.tsx
@@ -1,6 +1,6 @@
 "use client"
 
-import React, { Suspense } from "react"
+import React from "react"
 import { Container, styled, theme } from "ol-components"
 import HeroSearch from "@/page-components/HeroSearch/HeroSearch"
 import BrowseTopicsSection from "./BrowseTopicsSection"
@@ -52,25 +52,21 @@ const HomePage: React.FC = () => {
         <StyledContainer>
           <HeroSearch />
           <section>
-            <Suspense>
-              <FeaturedCoursesCarousel
-                titleComponent="h2"
-                title="Featured Courses"
-                config={carousels.FEATURED_RESOURCES_CAROUSEL}
-              />
-            </Suspense>
+            <FeaturedCoursesCarousel
+              titleComponent="h2"
+              title="Featured Courses"
+              config={carousels.FEATURED_RESOURCES_CAROUSEL}
+            />
           </section>
         </StyledContainer>
       </FullWidthBackground>
       <PersonalizeSection />
       <Container component="section">
-        <Suspense>
-          <MediaCarousel
-            titleComponent="h2"
-            title="Media"
-            config={carousels.MEDIA_CAROUSEL}
-          />
-        </Suspense>
+        <MediaCarousel
+          titleComponent="h2"
+          title="Media"
+          config={carousels.MEDIA_CAROUSEL}
+        />
       </Container>
       <BrowseTopicsSection />
       <TestimonialsSection />

diff --git a/frontends/main/src/app/departments/page.tsx b/frontends/main/src/app/departments/page.tsx
@@ -1,15 +1,27 @@
 import React from "react"
 import { Metadata } from "next"
-
+import DepartmentListingPage from "@/app-pages/DepartmentListingPage/DepartmentListingPage"
 import { standardizeMetadata } from "@/common/metadata"
+import { Hydrate } from "@tanstack/react-query"
+import { learningResourcesKeyFactory } from "api/hooks/learningResources"
+import { channelsKeyFactory } from "api/hooks/channels"
+import { prefetch } from "api/ssr/prefetch"
+
 export const metadata: Metadata = standardizeMetadata({
   title: "Departments",
 })
 
-import DepartmentListingPage from "@/app-pages/DepartmentListingPage/DepartmentListingPage"
+const Page: React.FC = async () => {
+  const dehydratedState = await prefetch([
+    channelsKeyFactory.countsByType("department"),
+    learningResourcesKeyFactory.schools(),
+  ])
 
-const Page: React.FC = () => {
-  return <DepartmentListingPage />
+  return (
+    <Hydrate state={dehydratedState}>
+      <DepartmentListingPage />
+    </Hydrate>
+  )
 }
 
 export default Page