Skip to content
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

fix: allow next.config.js overwrite cache-control header #39707

Closed
wants to merge 1 commit into from

Conversation

u3u
Copy link

@u3u u3u commented Aug 18, 2022

Update (2022-12-31): My Solution

Before the PR merge, if anyone else is bothered by it, my solution is to modify node_modules/next/dist/server/send-payload/revalidate-headers.js and then use patch-package to generate the patch. If you use pnpm then you can just use pnpm patch next to generate the patch using pnpm patch-commit after making changes in the temporary directory.

Fixes #21827, fixes #22319 #28557

https://nextjs.org/docs/api-reference/next.config.js/headers#cache-control

You cannot set Cache-Control headers in next.config.js file as these will be overwritten in production to ensure that API Routes and static assets are cached effectively.

In the documentation it is described that Cache-Control cannot be overridden and will affect static resources, but in fact static resources will set their own Cache-Control, so it does not seem to affect them.

match: getPathMatch('/_next/static/:path*'),
type: 'route',
name: '_next/static catchall',
fn: async (req, res, params, parsedUrl) => {
// make sure to 404 for /_next/static itself
if (!params.path) {
await this.render404(req, res, parsedUrl)
return {
finished: true,
}
}
if (
params.path[0] === CLIENT_STATIC_FILES_RUNTIME ||
params.path[0] === 'chunks' ||
params.path[0] === 'css' ||
params.path[0] === 'image' ||
params.path[0] === 'media' ||
params.path[0] === this.buildId ||
params.path[0] === 'pages' ||
params.path[1] === 'pages'
) {
this.setImmutableAssetCacheControl(res)
}

I use getHeader('Cache-Control') to check if the user has set their cache, which by default is the same behavior as before.
By the way hasHeader is undefined in some cases, I don't know why. (#34929 #37338)

@juulieen
Copy link

I would be interested in the merge of this PR too 👍

@bejarano-tech
Copy link

Fixes #21827, fixes #22319 #28557

https://nextjs.org/docs/api-reference/next.config.js/headers#cache-control

You cannot set Cache-Control headers in next.config.js file as these will be overwritten in production to ensure that API Routes and static assets are cached effectively.

In the documentation it is described that cache-control cannot be overridden and will affect static resources, but in fact static resources will set their own cache-control, so it does not seem to affect them.

match: getPathMatch('/_next/static/:path*'),
type: 'route',
name: '_next/static catchall',
fn: async (req, res, params, parsedUrl) => {
// make sure to 404 for /_next/static itself
if (!params.path) {
await this.render404(req, res, parsedUrl)
return {
finished: true,
}
}
if (
params.path[0] === CLIENT_STATIC_FILES_RUNTIME ||
params.path[0] === 'chunks' ||
params.path[0] === 'css' ||
params.path[0] === 'image' ||
params.path[0] === 'media' ||
params.path[0] === this.buildId ||
params.path[0] === 'pages' ||
params.path[1] === 'pages'
) {
this.setImmutableAssetCacheControl(res)
}

I use getHeader('Cache-Control') to check if the user has set their cache, which by default is the same behavior as before. By the way hasHeader is undefined in some cases, I don't know why. (#34929 #37338)

Hi, hasHeader is undefined when you have fallback: false in getStaticPaths

@meotimdihia
Copy link

@ijjk this feature is important for everyone who uses self-hosted + Cloudflare.

@benkingcode
Copy link

Any movement on this?

@ijjk ijjk requested a review from feedthejim as a code owner March 2, 2023 23:39
stefanobrambilla

This comment was marked as off-topic.

@cweekly
Copy link

cweekly commented May 25, 2023

Maintainers, please, could you review this change?

@mgray-bemyduo
Copy link

@timneutkens @stefanobrambilla @ijjk @shuding @huozhi @feedthejim
Can one of you please review this? It's been sitting here for almost a year and this bug is open because of it #22319
next-intl is not able to be used with server components until this is resolved. Please see here: https://next-intl-docs.vercel.app/docs/getting-started/app-router-server-components

@khalilsarwari
Copy link

This is a relatively small, opt-in change with huge usability benefits; both the issue and the solution are straightforward, can this please be prioritized?

@timneutkens @ijjk @huozhi

@a463
Copy link

a463 commented Aug 19, 2024

This is still an issue. I would like to cache ISR pages locally, but not on the CDN. There seems to be no way to overwrite the Cache-Control header. Could this please be included with Next 15? It seems like a relatively simple change.

@kmrachko
Copy link

kmrachko commented Sep 4, 2024

Adding one more voice to the crowd of people affected by that issue - please review it and either provide a clear explanation on why it's undesirable - or get it merged.

@ijjk ijjk closed this in a916dfc Sep 9, 2024
ijjk added a commit that referenced this pull request Sep 9, 2024
This continues #39707 bringing the
changes up to date with canary and adds test cases to ensure it's
working as expected.

Closes: #22319
Closes: #39707
Closes: NDX-148
Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Hi, went ahead and landed this against latest changes with added tests in #69802 thanks for looking into this!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet