Skip to content

Server render homepage and listing pages. Client-only drawer navigation. #1872

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 39 commits into from
Dec 6, 2024

Conversation

jonkafton
Copy link
Contributor

@jonkafton jonkafton commented Dec 3, 2024

What are the relevant tickets?

Closes:
Next.js: Homepage: Prefetch API calls #5920
Next.js: Topics, Units and Channels pages: Prefetch API calls #6036
Pages with a resource drawer should not call base page server render #6163

Description (What does it do?)

This reinstates PR #1847, which was reverted due an issue where opening the resource drawer triggers a call to the base server page for a partial rendering update, resulting in a refetch of all API content and subsequent performance issue and content shift as the already visible featured courses are reshuffled on the server. This also interfered with the click handlers on the resource cards, preventing the drawer from reopening.

The fix is to open and close the drawer by navigating using client side history.pushState().

The Cards now use ol-components/Link for shallow (ie. client-only) navigation so we can encapsulate the additional functionality there.

How can this be tested?

  • Testing for Server rendering for homepage, units and topics listing pages #1847 was done in that PR to commit 1cf3a38, but check for any regression (same head branch here with additional commit 0b0784).
  • Ensure that on the homepage, search page and pages with resource drawers that the drawer opens and closes without any calls for RSC payloads made to the server (e.g. with ?_rsc=1wtp7).
  • Ensure that on the homepage, the featured course carousel does not reshuffle and the hero image does not change when the drawer is opened or closed.

Additional Context

Spent some time getting a fully server rendered drawer to work (branch: https://github.com/mitodl/mit-learn/tree/jk/5920-prefetch-homepage-w-drawer-routing), but we landed on a decision to handle on the client given the code overhead and various trade offs needed. So this hit an apparent limitation of Next.js whereby navigation to different URL search params, which we use to trigger drawer open, results in a call to the server for a new RSC payload. This calls the base server page where we are not able to identify that the request is for only a partial and not a full render. In any case, as we make calls to prefetch API content on the base server page, any conditions here could break the server-client rendering synchronization and cause a hydration mismatch.

Next.js does provide mechanisms for server rending modal views, such as our drawer, using a combination of parallel and intercepting routes, however these must be implemented using the prescriptive filesystem based router which does not lend itself well to modals that need to be reused on many routes. The route slot, @Drawer, needed to be used with intercepting routes, (.), resulting in a file structure that looks like below, where the drawer "server page" plus error page and default route must be repeated for all routes with a drawer. The catch all segments do not appear to be useful here and can only be applied to subsequent segments, while we'd need to match on the first segments for e.g. /resource/1234 to be append-able. The structures below would also need to be added for the search page and the dashboard and any other routes we might add that need a drawer:

image

The user visiting a drawer open page would benefit from a server rendered drawer as it would open with rendered content, however a user opening a drawer on a page already loaded would in any case need to wait for the server round trip for content, either deferring the open animation or displaying the loading skeleton as we do currently, so the trade off with rendering on the client only against code complexity is fairly minimal.

Summary and other issues with a server rendered drawer:

  • Parallel route slots need to use paths defined by filesystem, so we can’t use search params - we need to replace ?resource=1234 with /resource/1234 appended to each path with a drawer.
  • …as a result, the parallel route server page structure needs to be repeated in all pages that have a drawer (not pretty, though we can have thin pages that re-export).
  • We get warnings on prefetches for the open drawer as the cache is checked before the drawer render - we’d need a way to defer the check.
  • Back/forward navigation does not animate the drawer (likely fixable with close first then navigate on transition end).
  • Closing the drawer does not update the metadata to the base page as we are wanting it to not call the page (open is fine).
    default.tsx needed alongside any @ slots to re-export the actual page.

@jonkafton jonkafton added the Needs Review An open Pull Request that is ready for review label Dec 4, 2024
@jonkafton jonkafton changed the title Server render homepage and channel pages. Client-only drawer navigation. Server render homepage and listing pages. Client-only drawer navigation. Dec 4, 2024
@ChristopherChudzicki ChristopherChudzicki self-assigned this Dec 4, 2024
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

👍


/* Pass shallow to navigate with window.history.pushState
* on the client only to prevent calls to the Next.js server
* for RSC payloads - these cause performance and hydration mismatch
Copy link
Contributor

Choose a reason for hiding this comment

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

The hydration mismatch is only for randomized APIs, right?

My understanding is that history.pushState / replaceState will only work for updating search params. I'd at least note that her, and possibly add

if (process.env.NODE_ENV === "development") {
  invariant(href.startsWith("?"), "Shallow routing can only be used to update search params")
}

Though I suppose the path could be the same as the current path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hydration mismatch is only for randomized APIs, right?

Here just making the assumption that any API responses will change, even if not frequently.

My understanding is that history.pushState / replaceState will only work for updating search params.

In theory, though we don't have any case for navigating to new paths on the client - I've added the invariant check.

@ChristopherChudzicki ChristopherChudzicki added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Dec 4, 2024
} else {
// Prevent scroll to top of page
router.push(`?${newParams}`, { scroll: false })
window.history.pushState({}, "", `?${newParams}${hash}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

The two conditionals are the same now. Let's get rid of one.

@jonkafton jonkafton force-pushed the jk/5920-prefetch-homepage branch from d03e2f6 to 301e023 Compare December 5, 2024 10:44
@jonkafton jonkafton merged commit 51710b3 into main Dec 6, 2024
11 checks passed
@odlbot odlbot mentioned this pull request Dec 9, 2024
15 tasks
@rhysyngsun rhysyngsun deleted the jk/5920-prefetch-homepage branch February 7, 2025 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants