Skip to content

Server rendering for homepage, units and topics listing pages #1847

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 36 commits into from
Nov 25, 2024

Conversation

jonkafton
Copy link
Contributor

@jonkafton jonkafton commented Nov 22, 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

Description (What does it do?)

This PR reinstates #1822, which was approved/merged but reverted as it included cache invalidation for learning resources to refetched them in the client to get the list memberships that did not work for all cases. This PR removes the need for it: #1846 as it fetches memberships independently from the learning resources.

Homepage

Makes API calls needed to render homepage learning resources on the server so that these prerender:

  • Featured Courses carousel "All"
  • Featured Courses carousel "Free"
  • Featured Courses carousel "With Certificate"
  • Featured Courses carousel "Professional & Executive Learning"
  • Media carousel "All"
  • Media carousel "Videos"
  • Media carousel "Podcasts"
  • Browse by Topic section

Unit Listing page /units

Updates to make a single call /api/v0/channels/?channel_type=unit, replacing calls to each channel detail page in the unit cards. Prefetches channels list and count by unit.

Topics Listing page /topics

Prefetches topics list and counts by topic.

How can this be tested?

These routes should server render all public content without any prefetch mismatch warnings in the browser console:

  • /
  • /units
  • /topics

Pages should render on first load with content with JavaScript disabled in the dev tools debugger settings.

Additional Context

Also adds reusable server PageParams type and cleans up the api key factory imports.

@jonkafton jonkafton added the Needs Review An open Pull Request that is ready for review label Nov 22, 2024
@ChristopherChudzicki ChristopherChudzicki self-assigned this Nov 25, 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.

👍

Comment: I think we should do...something... with frontends/main/src/app/getQueryClient.ts. It talks about its behavior for SSR, but is not used for SSR.

Do you know if the query client config matters when query client is used on server?

@jonkafton
Copy link
Contributor Author

jonkafton commented Nov 25, 2024

I don't think so given they're configuring cache behavior and retry etc, whereas on the server our query client is throwaway at the point we dehydrate for each request. It would be useful though to have a server side query client singleton that is scoped to the request. Next.js doesn't seem to have any way to access the request object outside of the middleware file to code for the request/response lifecycle.

@jonkafton jonkafton merged commit f2758b7 into main Nov 25, 2024
11 checks passed
@jonkafton jonkafton deleted the jk/5920-prefetch-homepage branch November 25, 2024 15:43
@odlbot odlbot mentioned this pull request Nov 25, 2024
19 tasks
@jonkafton jonkafton removed the Needs Review An open Pull Request that is ready for review label Dec 2, 2024
@jonkafton jonkafton restored the jk/5920-prefetch-homepage branch December 2, 2024 13:12
jonkafton added a commit that referenced this pull request Dec 2, 2024
jonkafton added a commit that referenced this pull request Dec 2, 2024
mbertrand pushed a commit that referenced this pull request Dec 2, 2024
* 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

* Prefetch homepage. Invalidate learning resource cache items

* Remove comment

* Update comment

* Temp remove featured resource list shuffle

* Remove unused

* Prefetch calls

* Prefetch topics page

* Rename key factory re-exports

* Units page prefetch

* Single request for all unit channel details

* Update unit listing page tests

* Page arg types

* Optional route params

* Remove comment

* Remove unused

* Remove comment

* Reinstate featured list shuffle and remove cache invalidation to refetch resources for user

* Add news to server render
@odlbot odlbot mentioned this pull request Dec 5, 2024
12 tasks
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