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.

Sign up for GitHub

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

next/image: Preserve Cache-Control headers if previously set #21072

Closed
wants to merge 12 commits into from
Closed

next/image: Preserve Cache-Control headers if previously set #21072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2bacfe9
next/image: Preserve Cache-Control headers if previously enforced
Jan 13, 2021
097ce2e
Merge branch 'canary' into respect-cache-headers
xavimb Jan 14, 2021
e666293
Merge branch 'canary' into respect-cache-headers
xavimb Jan 22, 2021
730d265
Merge branch 'canary' into respect-cache-headers
xavimb Jan 22, 2021
8f5af19
Merge branch 'canary' into respect-cache-headers
xavimb Feb 11, 2021
8adbf95
Merge branch 'canary' into respect-cache-headers
xavimb Feb 27, 2021
a46cf10
Merge branch 'canary' into respect-cache-headers
xavimb Jun 14, 2021
8bd5bed
Merge branch 'canary' into respect-cache-headers
xavimb Jun 17, 2021
1323523
Merge branch 'canary' into respect-cache-headers
xavimb Jun 22, 2021
d8aa32d
Merge branch 'canary' into respect-cache-headers
xavimb Jun 26, 2021
173a383
Merge branch 'canary' into respect-cache-headers
xavimb Jul 3, 2021
1be90b3
Merge branch 'canary' into respect-cache-headers
styfle Jul 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions packages/next/server/image-optimizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,12 +420,14 @@ function setResponseHeaders(
isDev: boolean
) {
res.setHeader('Vary', 'Accept')
res.setHeader(
'Cache-Control',
isStatic
? 'public, max-age=315360000, immutable'
: `public, max-age=${isDev ? 0 : maxAge}, must-revalidate`
)
if (!res.getHeader('Cache-Control')) {
res.setHeader(
'Cache-Control',
isStatic
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should allow overriding the static header. This can lead to accidentally de-optimization.

I would prefer PR #26739 and PR #23328 to handle this use case and avoid possible footguns.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @styfle , I agree that the solution you propose is neater. I'll close this PR in favour of the ones you mentioned. Thanks for your feedback and glad to see this issue is finally solved.

? 'public, max-age=315360000, immutable'
: `public, max-age=${isDev ? 0 : maxAge}, must-revalidate`
)
}
if (sendEtagResponse(req, res, etag)) {
// already called res.end() so we're finished
return { finished: true }
Expand Down
40 changes: 40 additions & 0 deletions test/integration/image-optimizer/test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,46 @@ describe('Image Optimizer', () => {
runTests({ w: size, isDev: false, domains })
})

describe('cache headers', () => {
beforeAll(async () => {
const newConfig = `{
async headers() {
return [
{
source: '/_next/image(.*)',
headers: [
{
key: 'Cache-Control',
value: 'public, max-age=180, s-maxage=180, stale-while-revalidate=180',
},
],
}
]
}
}`
await nextConfig.replace('{ /* replaceme */ }', newConfig)
appPort = await findPort()
app = await launchApp(appDir, appPort)
})
afterAll(async () => {
await killApp(app)
nextConfig.restore()
await fs.remove(imagesDir)
})
it('should not enforce cache control header if previously set', async () => {
const query = { url: '/test.png', w: 256, q: 80 }
const opts = { headers: { accept: 'image/webp' } }
const res = await fetchViaHTTP(appPort, '/_next/image', query, opts)
expect(res.status).toBe(200)
expect(res.headers.get('Content-Type')).toBe('image/webp')
expect(res.headers.get('cache-control')).toBe(
'public, max-age=180, s-maxage=180, stale-while-revalidate=180'
)
expect(res.headers.get('etag')).toBeTruthy()
await nextConfig.restore()
})
})

describe('dev support next.config.js cloudinary loader', () => {
beforeAll(async () => {
const json = JSON.stringify({
Expand Down