Mechanism to sync server prefetch with client API calls

[jonkafton]

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5918

Description (What does it do?)

To keep server and client API calls in sync, we need a solution that warns us if API calls are made from the browser to fetch content that could have been pre-rendered on the server, or indeed we're making unnecessary API calls on the server to fetch content that is not in the page. In addition to the warnings, it would ideally offer code reuse between server and client and help with the development workflow, or be minimally intrusive to it.

This PR issues console warnings for API calls made during the initial page render that have not been prefetched on the server and populated into the React Query cache.

• Waits for the document to be loaded and flags it on the query cache.
• Wraps our useQuery() hooks with a function that checks for cached data at a given key.
• Log a console warning if the document has not loaded and a key requested by a client component has nothing cached.
• Triggers a check on document load for any queries prefetched but not requested in components during the initial load.

Prefetches API data on the server for:

• some of the homepage (excludes resources, commented in code)
• the department listing page

How can this be tested?

• Do not prefetch a query that is needed during initial render, e.g. in frontends/main/src/app/departments/page.tsx, remove channelsKeyFactory.countsByType("department") from the prefetch array.

  In the consoles expect to see warning:

['channel', 'countsByType', 'department'] Content was not prefetched on the server for query key needed during initial render

• Prefetch a query on the server that is not needed for initial render, e.g. in frontends/main/src/app/departments/page.tsx add channelsKeyFactory.countsByType("random") to the prefetch array.

  In the consoles expect to see warning:

["channel","countsByType","random"] Content was prefetched on the server for query key not needed during initial render 

Additional Context

For discussion, comments below.

[jonkafton]

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

[jonkafton]

I tried a variety of approaches aiming at the initial goal of specifying the queries needed for a page in one place, prefetching all on the server, then having client components pull from this construct to be checked off; otherwise warnings logged. The issues here were first that queries are parameterized and often in waterfall, so we'd need a dependency graph that could pass results, but also had to be serializable so it could be shared between server and client.
Some sort of declaratively specified DAG with serializable state was quickly looking impractical and overkill for this purpose :)

An issue with listing out the queries at page level is also that the may be called at any point down the tree by any component that will often be in use across pages. Although conceivably components could repeatedly call usePageQueries() which would return all the results for the page (they are cached), keying them for reusable destructuring in code quickly gets messy.

Listing out the API calls in the components and exporting as arrays for use in server components would be good, though a server component cannot import anything other than a React node from a client component, plus we'd need to follow the component tree to aggregate all calls.

Currently we have a lightweight approach that has the benefit of needed no change to the client components (these continue to use the custom hooks), though does not give us code reuse in specifying the calls. In the page (server component), the key factory methods are called directly to prefetch into the query cache.

• Warnings are logged to the console but no fail fast / build time error.

• This adds the logic to a function that replaces useQuery(). This allows us to encapsulate it and only change the useQuery import to our own, without needing to wrap all the custom hook key factory calls.

• A problem here is that this does not cover the cases that we call the key factory methods directly, notably in ResourceCarousel, https://github.com/mitodl/mit-learn/blob/jk/5919-prefetch-mechanism/frontends/main/src/page-components/ResourceCarousel/ResourceCarousel.tsx#L202-L207.

• This is using the window load event to signal that initial rendering is complete. Do we have a more reliable way to determine that the DOM has resolved and we are no longer in cache detection mode. Ideally we'd have a clean cut off to delineate the initial server renderable calls without potential timing issues. The document DOMContentLoaded event which happens sooner seems to not make a difference for our purposes.

• In the tests we'll need to expect or ignore console warnings, or actually populate the cache.

• Prefetching the testimonials, for example, though server rendered, did not result in the carousel appearing quickly on load. There appears to be significant blocking time while the Slick library makes width calculations, the likely candidate for slow blocking time in performance results. Also appears to be interdependency between the carousels - if Featured Courses are also prefetched, testimonials appears immediately.

• As learning resources constitute a lot of the page content, prefetching provides a significant performance boost. Currently these include user specific data (subscription lists) and we have https://github.com/mitodl/hq/issues/5159 to address this. In the meantime it may be worthwhile prefetching on the server and then invalidating the cache on the client so that these are fetched again for the specific user. This would cause problems if the endpoints returned resources in a different order or with different data (an ingestion may have completed).

[ChristopherChudzicki]

First: I like the idea of warnings or logs. Good prompts for us.

As an alternative to a custom wrapper around useQueries, I want to propose inspecting the query cache after first render. I'm pretty sure it has enough information to provide the relevant warnings without customizing prefetch & useQuery.

Here's what I came up with:

useMissingPrefetchWarning based on first render query cache

const PREFETCH_EXEMPT_QUERIES = [
  ["userMe"],
  ["initialKeys"],
  learningResourcesKeyFactory.learningpaths._def,
  learningResourcesKeyFactory.userlists._def,
]
/**
 * Call this as high as possible in render tree to detect query usage on
 * first render.
 */
export const useMissingPrefetchWarning = (
  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 === "development") {
        const cache = queryClient.getQueryCache()
        const queries = cache.getAll()
        console.log("Queries at first render:")
        console.table(
          queries.map((query) => {
            return {
              hash: query.queryHash,
              disabled: query.isDisabled(),
              initialStatus: query.initialState.status,
              status: query.state.status,
            }
          }),
        )
        const exempted = exemptions.map((key) => cache.find(key))
        const potentialPrefetches = queries.filter(
          (query) =>
            !exempted.includes(query) &&
            query.initialState.status !== "success" &&
            !query.isDisabled(),
        )
        if (potentialPrefetches.length > 0) {
          console.log(
            "The following queries were requested in first render but not prefetched. ",
            "If these queries are user-specific, they cannot be prefetched. ",
            "Otherwise, consider fetching on the server with prefetch.",
          )
          potentialPrefetches.forEach((query) => {
            console.log(query.queryKey)
          })
        }
      }
    },
    // We only want to run this on initial render.
    // (Aside: queryClient should be a singleton anyway)
    // eslint-disable-next-line react-hooks/exhaustive-deps
    [],
  )
}
 

*Note: I made it a bit more verbose than we would probably want for the final version.*

The main advantage of this approach is no querying with a custom hook, so it works with both useQuery and useQueries out of the box.

The downside is that we need an exemption list, though the original approach essentially has that aspect, too. (exemptions = everything that uses out-of-the-box useQuery hook rather than customized version).

Oh, and I think it would be good to only run it in dev modee?

---

Separately Especially for some more complicated cases (like if we decide to prefetch search queries that depend on search parameters) might be nice to have a way for the server and client components to share the prefetches, like I tried to do in https://gist.github.com/ChristopherChudzicki/ab11931ace608d6ab4e36392c40a38e8 (but never got the types working quite right).

[jonkafton]

As an alternative to a custom wrapper around useQueries, I want to propose inspecting the query cache after first render.

This is a much cleaner approach to warning on any queries made on the client that have not been prefetched, but I've not been able to find a way to get it to work for the inverse without intercepting useQuery or otherwise wrapping the API methods - ie. warning on anything prefetched on the server that is not accessed during initial render. Both sides of the problem are perhaps equal in terms of impact to performance, particularly as the site grows and unnecessary queries become harder to spot.

• Hierarchical key matching is a nice bonus (and a lot cleaner than stringifying the keys for matching!)

• Why don't I use console.table() more!?

Separately Especially for some more complicated cases (like if we decide to prefetch search queries that depend on search parameters) might be nice to have a way for the server and client components to share the prefetches.

Totally - most my time looking at this has actually been looking for a solution here. Ideally we'd specify queries and the dependencies between them declaratively, which requires some sort of graph spec. The problem is also that an "active" list of queries with state and holding values needs to be serialized for it to be shared between server and client - it's starting to look over engineered for our purposes. I'm sure we could pass as props though and let Next.js handle serialization.

[jonkafton]

This is a much cleaner approach to warning on any queries made on the client that have not been prefetched, but I've not been able to find a way to get it to work for the inverse

Scratch that - the queries have an observer count.

[ChristopherChudzicki]

Thanks for the summary above. I had forgotten about the "warn on unnecessary prefetches" aspect. Glad observer count solves that.

An issue with listing out the queries at page level is also that the may be called at any point down the tree by any component that will often be in use across pages. Although conceivably components could repeatedly call usePageQueries() which would return all the results for the page (they are cached), keying them for reusable destructuring in code quickly gets messy.

Yeah. We're currently using the App router in a way that resembles Pages router, where all prefetching happens at the Page level. Presumably restructuring to use more server components could help with this, though that would be a big change (and might complicate our current testing approach).

Listing out the API calls in the components and exporting as arrays for use in server components would be good, though a server component cannot import anything other than a React node from a client component, plus we'd need to follow the component tree to aggregate all calls.

A possible workaround for that, btw, was having a separate queries.ts file per page, to be imported by both the client and the sserver component. But it does get complicated in the cases you mention.

---

All in all, I think the warning approach is pretty good / pragmatic.

[ChristopherChudzicki]

👍 New console messages are great.

[ChristopherChudzicki]

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.

[ChristopherChudzicki]

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

[ChristopherChudzicki]

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

[jonkafton]

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.