Skip to content

Conversation

@jonkafton
Copy link
Contributor

@jonkafton jonkafton commented Oct 16, 2024

What are the relevant tickets?

https://github.com/mitodl/hq/issues/5796

Description (What does it do?)

Next.js sets no-cache for any dynamically rendered pages, though we have taken the approach of ensuring all server rendered HTML is public and can be stored on shared caches.

This change overrides this behavior by setting cache headers explicitly for all of our routes.

How can this be tested?

Must be tested in Production mode.

Locally you should see Cache-Control: public, max-age=120 on all base HTML responses, but not on JS chunks, images or other static content.

Once deployed you should see CDN cache hits on subsequent requests (X-Cache header).

Additional Context

  • This also upgrades Next.js to 14.2.15 so that we have this recent change in v14.2.10 Add ability to customize Cache-Control vercel/next.js#69802.

  • Several routes are static rendered at build time (/about, /departments, /topics, /units, etc). Next.js gives these an s-maxage=31536000, though we're setting all to the same for consistency.

  • This does also set the cache header on RSC payloads e.g. /?resource=6135&_rsc=1wtp7. This may cause issues.

  • This only sets a short TTL of 120s - wanting to be cautious while we first test.

@jonkafton jonkafton added the Needs Review An open Pull Request that is ready for review label Oct 16, 2024
@jonkafton jonkafton force-pushed the jk/5796-cache-headers branch from 2a959f9 to 8ebc4ea Compare October 16, 2024 19:56
@ChristopherChudzicki ChristopherChudzicki self-assigned this Oct 16, 2024
Comment on lines +62 to +65
/* This is intended to target the base HTML responses. Some are dynamically rendered,
* so Next.js instructs no-cache, however we are currently serving public content that
* is cacheable. Excludes everything with a file extension so we're matching only on routes.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion is mention RSC here, too.

Suggested change
/* This is intended to target the base HTML responses. Some are dynamically rendered,
* so Next.js instructs no-cache, however we are currently serving public content that
* is cacheable. Excludes everything with a file extension so we're matching only on routes.
*/
/* This is intended to target the base HTML responses and streamed RSC
* content. Some routes are dynamically rendered, so NextJS by default
* sets no-cache. However we are currently serving public content that is
* cacheable.
*
* Excludes everything with a file extension so we're matching only on routes.
*/

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.

IMO both 120 and 600 could be much longer. Do you intend on increasing after testing?

Should we use s-maxage instead? My understanding is Fastly would respect that, but not browsers? (cc @blarghmatey ?)

@jonkafton
Copy link
Contributor Author

IMO both 120 and 600 could be much longer. Do you intend on increasing after testing?

Yes. I want to test the behavior before and after cache expiry for comparison. We can cache the images indefinitely assuming the cache purge on release works as expected. For the HTML perhaps half the duration we consider an API response to be fresh - 12 hours?

Should we use s-maxage instead? My understanding is Fastly would respect that, but not browsers?

I've updated to s-maxage as you're correct - we're only trying to instruct the shared cache. It's not 100% clear whether browsers would cache responses marked as public, though it seems intended more to override shared cache behavior for authenticated requests and not to instruct browsers.

@jonkafton jonkafton merged commit 00976e7 into nextjs Oct 17, 2024
12 checks passed
jonkafton added a commit that referenced this pull request Oct 17, 2024
@jonkafton jonkafton removed the Needs Review An open Pull Request that is ready for review label Oct 17, 2024
ChristopherChudzicki pushed a commit that referenced this pull request Oct 18, 2024
* Middleware to set cache headers

* Set headers in the config

* Upgrade Next.js

* Regex to exclude paths with a file extension to apply only to routes

* Remove middleware file

* Instruct to only store in shared cache (not browsers)
ChristopherChudzicki pushed a commit that referenced this pull request Oct 18, 2024
* Middleware to set cache headers

* Set headers in the config

* Upgrade Next.js

* Regex to exclude paths with a file extension to apply only to routes

* Remove middleware file

* Instruct to only store in shared cache (not browsers)
jonkafton added a commit that referenced this pull request Oct 21, 2024
* Merge `main` into `nextjs` (#1687)

* Add location field to LearningResource and LearningResourceRun models (#1604)

* Always delete past events during ETL, and filter them out from api results too just in case (#1660)

* Release 0.21.1

* Release date for 0.21.1

* remove unnecessary padding adjustment on Input base styles (#1666)

* remove unnecessary padding adjustment on Input base styles

* adjust actual textarea padding to match Figma

* Content File Score Adjustment (#1667)

* Release 0.21.2

* Restore program letter intercept view (#1643)

* Restore program letter intercept view

* Generate schema

* Fix test

* fixing import

* Revert "fixing import"

This reverts commit cf56807.

---------

Co-authored-by: shankar ambady <ambady@mit.edu>

* Release date for 0.21.2

* topic detail banner / subtopic logic revisions (#1646)

* add new darker chip variant

* move subtopic display to the banner

* remove max width on banner subtitle texts

* query subtopics only on parent topic channels, and on child topic channels query "related topics" aka the topic's siblings

* don't display the current topic in related topics

* show parent topic breadcrumbs on child topic channel pages

* fix tests

* only do subtopics mocking if necessary

* fix editchannelpage test

* move action buttons under the title on mobile

* fix test

* add new default channel background image

* "toopics"

* fix background position

* remove accidentally committed file

* fix background size on mobile

* remove unnecessary order css rule

* fix banner background on mobile

* fix breadcrumb props

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove unnecessary topic id filter

* remove unnecessary breadcrumb fallback

* set the default banner background image in the banner component itself, and only apply large and up 140% background size if the default image is being used

* properly set default background image

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Update Yarn to v4.5.0 (#1624)

* Update Yarn to v4.5.0

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Clear Cache on Deploy (#1668)

* adding initial cache clear command

* adding cache clear command to heroku release script

* fix docstrings

* removing invalid flag from clear cache command (#1675)

* prevent featured course carousel from re-randomizing (#1673)

* use skipFeatured when invalidating userlists / learning paths after update

* add a new function for updating query data for user_list_parents and learning_path_parents on the featured courses list when either is edited without re-randomizing

* fix learning path invalidations

* fix test

* Shuffling around where the search_update event is fired so it happens in more places (#1679)

- Changed HeroSearch to use the SearchField component instead of SearchInput, so we get the event capture stuff
- Updated SearchInput to not require setPage (for use on the homepage)
- Moved facet events out of the SearchPage and into SearchDisplay so they get called on other pages that use it
- Also trigger the facet events on more things - should also now get sorting changes, choosing between categories, and clearing facets

* Reinstate background steps image

---------

Co-authored-by: Matt Bertrand <mrbertrand@gmail.com>
Co-authored-by: Doof <mitx-devops@mit.edu>
Co-authored-by: Carey P Gumaer <gumaerc@mit.edu>
Co-authored-by: Anastasia Beglova <abeglova@mit.edu>
Co-authored-by: Nathan Levesque <rhysyngsun@users.noreply.github.com>
Co-authored-by: shankar ambady <ambady@mit.edu>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Shankar Ambady <shanbady@gmail.com>
Co-authored-by: James Kachel <james@jkachel.com>

* restoring up and down chevrons (#1690)

* Config to set cache control headers (#1700)

* Middleware to set cache headers

* Set headers in the config

* Upgrade Next.js

* Regex to exclude paths with a file extension to apply only to routes

* Remove middleware file

* Instruct to only store in shared cache (not browsers)

* Copy yarn releases to the Docker container (fixes build) (#1703)

* Copy yarn releases

* Build the docker image on all push

* NextJS - re-enable program letter tests (#1696)

* fixing program letter template

* re-enabling test

* adding space

* remove unnecessary webpack customizations (#1704)

* Increase cache duration (#1705)

* Increase cache durations

* Update comment

* Prod deployment. Add Posthog vars

* NextJS Sentry Integration (#1701)

* configure sentry, fix error pages

* add env vars to CI

* add global styles to global-error

* TEMPORARY self destruct button

* cleanup

* cleanup error boundary, other misc cleanup

* remove a few useless things

* add a simple test i guess

* remove intentional error triggers

* Appzi env vars and Sentry config

* Move deploy jobs to respective workflow. Docker build for dry run

---------

Co-authored-by: Matt Bertrand <mrbertrand@gmail.com>
Co-authored-by: Doof <mitx-devops@mit.edu>
Co-authored-by: Carey P Gumaer <gumaerc@mit.edu>
Co-authored-by: Anastasia Beglova <abeglova@mit.edu>
Co-authored-by: Nathan Levesque <rhysyngsun@users.noreply.github.com>
Co-authored-by: shankar ambady <ambady@mit.edu>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Shankar Ambady <shanbady@gmail.com>
Co-authored-by: James Kachel <james@jkachel.com>
Co-authored-by: Chris Chudzicki <christopher.chudzicki@gmail.com>
@odlbot odlbot mentioned this pull request Oct 22, 2024
74 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.

3 participants