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

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

Closed
wants to merge 12 commits into from

Conversation

xavimb
Copy link

@xavimb xavimb commented Jan 13, 2021

Next/image does currently enforce Cache-Control headers, as explained in this issue: #19914

This PR makes the Next/image component respect previously set cache-control headers. In case of having this next.config.json file:

module.export = {
  async headers() {
    return [
      {
        source: '/_next/image(.*)',
        headers: [
          {
            key: 'Cache-Control',
            value: 'public, max-age=180, s-maxage=180, stale-while-revalidate=180',
          },
        ],
      }
    ]
  }
}

All images will return the Cache-Control header set in the configuration file instead of being overwritten to public, max-age=0, must-revalidate.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@xavimb xavimb changed the title next/image: Preserve Cache-Control headers if previously enforced next/image: Preserve Cache-Control headers if previously set Jan 14, 2021
@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@xavimb xavimb requested a review from divmain as a code owner February 27, 2021 08:26
@ijjk

This comment has been minimized.

@FDiskas
Copy link

FDiskas commented Mar 24, 2021

Actually nextjs works as is documented - so in your case original images should have corect headers before processing them with next/image lib - in case your original images is in public directory you can pass headers like so:

  async headers() {
    return [
      {
        source: '/:all*(svg|jpg|png)',
        locale: false,
        headers: [
          {
            key: 'Cache-Control',
            value: 'public, max-age=31540000, must-revalidate',
          }
        ],
      },
    ]
  },

in this case headers still be Cache-Control: public, max-age=0, must-revalidate but Status Code: 304 Not Modified will be also available - that means for the browser that he should reuse cached version. In screenshot belo you will se that original image in my case is 1MB but actual received data is less
image

@xavimb
Copy link
Author

xavimb commented Mar 24, 2021

There are multiple cases where we want to leverage caching the image on the browser or in a CDN, without having to do an extra request to see if the image has changed. As you can see in your example, it takes 243ms to load the image, even if it hasn't changed. In the case of in-browser caching, this is a sub-millisecond operation that can speed up page load times by a lot when there are multiple images.

@FDiskas
Copy link

FDiskas commented Mar 24, 2021

That would help probably #23328

@xavimb
Copy link
Author

xavimb commented Mar 24, 2021

The #23328 PR allows changing the max-age of the images cached inside Nextjs and it overrides the cache-control header from the upstream. That is useful when you don't have control over the upstream cache headers that your upstream sets, so that you can control for how long do you want to cache those images inside Nextjs. Even when modifying the max-age, you still don't fix the problem that I'm trying to solve, that's that browsers need to always do a request to fetch (or revalidate) the image every single time you load an image.

So this PR addresses something different than #23328. This PR is about being able to cache images in browsers and CDNs, and it doesn't do anything regarding the images cached in Nextjs nor the upstream images.

@FDiskas
Copy link

FDiskas commented Mar 24, 2021

@xavimb - thank you for great explanation - now I get the point and totally agree with you and your PR.
Lets subscribe and see how it goes. Hope it will be merged soon.

Also it will prevent additional requests to nextjs and slightly increase overall performance and decrease memory usage.

@TheThirdRace
Copy link

@xavimb Thanks for the PR, it's exactly what a lot of people are waiting for!

Is there a way to check that PR again, there's a conflict and some tests failed for apparently no reason...

Anyone from NextJs could help expedite this PR please? This is critical for many people and the wait has been way too long for this fix. @timneutkens @Timer @ijjk (sorry to ping directly like this, just let me know if there's an easier way to get attention to a PR)

@sebfie
Copy link

sebfie commented Jun 14, 2021

I agree, we really need it !!

@xavimb xavimb requested a review from shuding as a code owner June 14, 2021 17:15
# Conflicts:
#	packages/next/next-server/server/image-optimizer.ts
@xavimb xavimb force-pushed the respect-cache-headers branch from 3a960bd to a46cf10 Compare June 14, 2021 17:36
@xavimb xavimb requested review from huozhi, padmaia and styfle as code owners July 3, 2021 06:17
@ijjk

This comment has been minimized.

@ijjk
Copy link
Member

ijjk commented Jul 6, 2021

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary xavimb/next.js respect-cache-headers Change
buildDuration 12.7s 12.5s -184ms
buildDurationCached 3s 2.9s -72ms
nodeModulesSize 49.3 MB 49.3 MB ⚠️ +157 B
Page Load Tests Overall increase ✓
vercel/next.js canary xavimb/next.js respect-cache-headers Change
/ failed reqs 0 0
/ total time (seconds) 2.374 2.287 -0.09
/ avg req/sec 1053.18 1093.31 +40.13
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.546 1.433 -0.11
/error-in-render avg req/sec 1617.55 1744.47 +126.92
Client Bundles (main, webpack, commons)
vercel/next.js canary xavimb/next.js respect-cache-headers Change
359.HASH.js gzip 3.09 kB 3.09 kB
framework-HASH.js gzip 42 kB 42 kB
main-HASH.js gzip 20.6 kB 20.6 kB
webpack-HASH.js gzip 1.49 kB 1.49 kB
Overall change 67.2 kB 67.2 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary xavimb/next.js respect-cache-headers Change
polyfills-HASH.js gzip 31.1 kB 31.1 kB
Overall change 31.1 kB 31.1 kB
Client Pages
vercel/next.js canary xavimb/next.js respect-cache-headers Change
_app-HASH.js gzip 803 B 803 B
_error-HASH.js gzip 3.18 kB 3.18 kB
amp-HASH.js gzip 526 B 526 B
css-HASH.js gzip 329 B 329 B
hooks-HASH.js gzip 903 B 903 B
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 1.65 kB 1.65 kB
routerDirect..HASH.js gzip 322 B 322 B
withRouter-HASH.js gzip 320 B 320 B
bb14e60e810b..30f.css gzip 125 B 125 B
Overall change 8.42 kB 8.42 kB
Client Build Manifests
vercel/next.js canary xavimb/next.js respect-cache-headers Change
_buildManifest.js gzip 390 B 390 B
Overall change 390 B 390 B
Rendered Page Sizes
vercel/next.js canary xavimb/next.js respect-cache-headers Change
index.html gzip 523 B 523 B
link.html gzip 537 B 537 B
withRouter.html gzip 515 B 515 B
Overall change 1.57 kB 1.57 kB

Webpack 4 Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary xavimb/next.js respect-cache-headers Change
buildDuration 10.7s 10.8s ⚠️ +85ms
buildDurationCached 4.4s 4.2s -189ms
nodeModulesSize 49.3 MB 49.3 MB ⚠️ +157 B
Page Load Tests Overall increase ✓
vercel/next.js canary xavimb/next.js respect-cache-headers Change
/ failed reqs 0 0
/ total time (seconds) 2.326 2.331 0
/ avg req/sec 1074.61 1072.67 ⚠️ -1.94
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.502 1.495 -0.01
/error-in-render avg req/sec 1664.11 1671.84 +7.73
Client Bundles (main, webpack, commons)
vercel/next.js canary xavimb/next.js respect-cache-headers Change
14.HASH.js gzip 3.11 kB 3.11 kB
677f882d2ed8..HASH.js gzip 13.9 kB 13.9 kB
framework.HASH.js gzip 41.8 kB 41.8 kB
main-HASH.js gzip 7.81 kB 7.81 kB
webpack-HASH.js gzip 1.19 kB 1.19 kB
Overall change 67.8 kB 67.8 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary xavimb/next.js respect-cache-headers Change
polyfills-HASH.js gzip 31.3 kB 31.3 kB
Overall change 31.3 kB 31.3 kB
Client Pages
vercel/next.js canary xavimb/next.js respect-cache-headers Change
_app-HASH.js gzip 791 B 791 B
_error-HASH.js gzip 3.83 kB 3.83 kB
amp-HASH.js gzip 531 B 531 B
css-HASH.js gzip 333 B 333 B
hooks-HASH.js gzip 910 B 910 B
index-HASH.js gzip 227 B 227 B
link-HASH.js gzip 1.64 kB 1.64 kB
routerDirect..HASH.js gzip 295 B 295 B
withRouter-HASH.js gzip 292 B 292 B
e025d2764813..52f.css gzip 125 B 125 B
Overall change 8.98 kB 8.98 kB
Client Build Manifests
vercel/next.js canary xavimb/next.js respect-cache-headers Change
_buildManifest.js gzip 418 B 418 B
Overall change 418 B 418 B
Rendered Page Sizes
vercel/next.js canary xavimb/next.js respect-cache-headers Change
index.html gzip 569 B 569 B
link.html gzip 581 B 581 B
withRouter.html gzip 561 B 561 B
Overall change 1.71 kB 1.71 kB
Commit: 1be90b3

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.

@xavimb xavimb closed this Jul 6, 2021
@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants