Skip to content

Mechanism to sync server prefetch with client API calls #1798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Nov 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions frontends/api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
Expand Down
2 changes: 2 additions & 0 deletions frontends/api/src/hooks/channels/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const useChannelDetail = (channelType: string, channelName: string) => {
...channels.detailByType(channelType, channelName),
})
}

const useChannelCounts = (channelType: string) => {
return useQuery({
...channels.countsByType(channelType),
Expand Down Expand Up @@ -54,4 +55,5 @@ export {
useChannelsList,
useChannelPartialUpdate,
useChannelCounts,
channels as channelsKeyFactory,
}
8 changes: 1 addition & 7 deletions frontends/api/src/hooks/learningResources/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -538,4 +531,5 @@ export {
useListItemMove,
usePlatformsList,
useSchoolsList,
learningResources as learningResourcesKeyFactory,
}
7 changes: 6 additions & 1 deletion frontends/api/src/hooks/newsEvents/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,9 @@ const useNewsEventsDetail = (id: number) => {
return useQuery(newsEvents.detail(id))
}

export { useNewsEventsList, useNewsEventsDetail, NewsEventsListFeedTypeEnum }
export {
useNewsEventsList,
useNewsEventsDetail,
NewsEventsListFeedTypeEnum,
newsEvents as newsEventsKeyFactory,
}
6 changes: 5 additions & 1 deletion frontends/api/src/hooks/testimonials/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,8 @@ const useTestimonialDetail = (id: number | undefined) => {
})
}

export { useTestimonialDetail, useTestimonialList }
export {
useTestimonialDetail,
useTestimonialList,
testimonials as testimonialsKeyFactory,
}
3 changes: 2 additions & 1 deletion frontends/api/src/hooks/widget_lists/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down
13 changes: 13 additions & 0 deletions frontends/api/src/ssr/prefetch.ts
Original file line number Diff line number Diff line change
@@ -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)
}
118 changes: 118 additions & 0 deletions frontends/api/src/ssr/usePrefetchWarnings.test.ts
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not tried, but I assume you could also just do a real prefetch. We mock the axios call, not anything react-query related.

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"],
)
})
})
96 changes: 96 additions & 0 deletions frontends/api/src/ssr/usePrefetchWarnings.ts
Original file line number Diff line number Diff line change
@@ -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,
Comment on lines +12 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you, but we could remove status. Since we're showing "initialStatus" and "status at first render" I think they are the same?

observerCount: query.getObserversCount(),
})),
["hash", "initialStatus", "status", "observerCount", "disabled"],
)
}

const PREFETCH_EXEMPT_QUERIES = [["userMe"]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, maybe we will want to add the userlist membership query soon to exemptions list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any way of marking any user specific vs public queries? There is a meta object on the query config, though ideally we'd have some rule. Not so easy unless there's some indicator in the API design itself, e.g. all public endpoints live under /api/v1/public.


/**
* 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
[],
)
}
26 changes: 11 additions & 15 deletions frontends/main/src/app-pages/HomePage/HomePage.tsx
Original file line number Diff line number Diff line change
@@ -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"
Expand Down Expand Up @@ -52,25 +52,21 @@ const HomePage: React.FC = () => {
<StyledContainer>
<HeroSearch />
<section>
<Suspense>
Copy link
Contributor Author

@jonkafton jonkafton Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer seeing the "useSearchParams() should be wrapped in a suspense boundary" error?!

<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 />
Expand Down
20 changes: 16 additions & 4 deletions frontends/main/src/app/departments/page.tsx
Original file line number Diff line number Diff line change
@@ -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
Loading
Loading