feat(images): use experimental isrFlushToDisk option to prevent writing optimized images to cache
#70645
+77
−25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ijjk
added
Documentation
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
styfle
reviewed
Sep 30, 2024
styfle
reviewed
Sep 30, 2024
test/integration/image-optimizer/test/disable-write-to-cache-dir.test.ts
Show resolved
Hide resolved
|
Thanks for working on this! I'm running CI again. Can you also update the PR title and description? |
Failing test suitesCommit: f4a915b
Expand output● with dangerouslyAllowSVG config › dev support with next.config.js › should use cache and stale-while-revalidate when query is the same for external image Read more about building and testing Next.js in contributing.md.
Expand output● with contentDispositionType inline › dev support with next.config.js › should use cache and stale-while-revalidate when query is the same for external image ● with contentDispositionType inline › Production Mode Server support with next.config.js › should use cache and stale-while-revalidate when query is the same for external image Read more about building and testing Next.js in contributing.md.
Expand output● app-dir-hmr › filesystem changes › should update server components pages when env files is changed (node) Read more about building and testing Next.js in contributing.md.
Expand output● with minimumCacheTTL of 5 sec › dev support with next.config.js › should use cache and stale-while-revalidate when query is the same for external image ● with minimumCacheTTL of 5 sec › Production Mode Server support with next.config.js › should use cache and stale-while-revalidate when query is the same for external image Read more about building and testing Next.js in contributing.md.
Expand output● with latest sharp › dev support with next.config.js › should use cache and stale-while-revalidate when query is the same for external image ● with latest sharp › Production Mode Server support with next.config.js › should use cache and stale-while-revalidate when query is the same for external image Read more about building and testing Next.js in contributing.md.
Expand output● i18n Support › production mode › should 404 for locale prefixed static assets correctly ● i18n Support › production mode › should serve public file on locale domain ● i18n Support › production mode › should 404 for locale prefixed public folder files ● i18n Support › production mode › should redirect external domain correctly ● i18n Support › production mode › should have domainLocales available on useRouter ● i18n Support › production mode › should not error with similar named cookie to locale cookie ● i18n Support › production mode › should not add duplicate locale key when navigating back to root path with query params ● i18n Support › production mode › should not add duplicate locale key when navigating back to root path with hash ● i18n Support › production mode › should handle navigating back to different casing of locale ● i18n Support › production mode › should have correct initial query values for fallback ● i18n Support › production mode › should navigate to page with same name as development buildId ● i18n Support › production mode › should redirect to locale domain correctly client-side ● i18n Support › production mode › should render the correct href for locale domain ● i18n Support › production mode › should prerender with the correct href for locale domain ● i18n Support › production mode › should render the correct href with locale domains but not on a locale domain ● i18n Support › production mode › should navigate through history with query correctly ● i18n Support › production mode › should not contain backslashes in pages-manifest ● i18n Support › production mode › should resolve href correctly when dynamic route matches locale prefixed ● i18n Support › production mode › should use default locale when no locale is in href with locale false ● i18n Support › production mode › should preload all locales data correctly ● i18n Support › production mode › should have correct values for non-prefixed path ● i18n Support › production mode › should not have hydration mis-match from hash ● i18n Support › production mode › should add i18n config to routes-manifest ● i18n Support › production mode › should output correct prerender-manifest ● i18n Support › production mode › should resolve auto-export dynamic route correctly ● i18n Support › production mode › should navigate to auto-export dynamic page ● i18n Support › production mode › should apply trailingSlash redirect correctly ● i18n Support › production mode › should apply redirects correctly ● i18n Support › production mode › should apply headers correctly ● i18n Support › production mode › should visit API route directly correctly ● i18n Support › production mode › should visit dynamic API route directly correctly ● i18n Support › production mode › should rewrite to API route correctly for en-US locale ● i18n Support › production mode › should rewrite to API route correctly for nl-NL locale ● i18n Support › production mode › should rewrite to API route correctly for nl-BE locale ● i18n Support › production mode › should rewrite to API route correctly for nl locale ● i18n Support › production mode › should rewrite to API route correctly for fr-BE locale ● i18n Support › production mode › should rewrite to API route correctly for fr locale ● i18n Support › production mode › should rewrite to API route correctly for en locale ● i18n Support › production mode › should rewrite to API route correctly for go locale ● i18n Support › production mode › should rewrite to API route correctly for go-BE locale ● i18n Support › production mode › should rewrite to API route correctly for do locale ● i18n Support › production mode › should rewrite to API route correctly for do-BE locale ● i18n Support › production mode › should apply rewrites correctly ● i18n Support › production mode › should navigate with locale prop correctly ● i18n Support › production mode › should navigate with locale prop correctly GSP ● i18n Support › production mode › should navigate with locale false correctly ● i18n Support › production mode › should navigate with locale false correctly GSP ● i18n Support › production mode › should update asPath on the client correctly ● i18n Support › production mode › should handle fallback correctly after generating ● i18n Support › production mode › should use correct default locale for locale domains ● i18n Support › production mode › should not strip locale prefix for default locale with locale domains ● i18n Support › production mode › should not redirect to accept-lang preferred locale with locale cookie ● i18n Support › production mode › should redirect to correct locale domain ● i18n Support › production mode › should handle locales with domain › for domain example.do › should handle locale do-BE ● i18n Support › production mode › should handle locales with domain › for domain example.do › should handle locale go-BE ● i18n Support › production mode › should handle locales with domain › for domain example.com › should handle locale do-BE ● i18n Support › production mode › should handle locales with domain › for domain example.com › should handle locale go-BE ● i18n Support › production mode › should provide defaultLocale correctly for locale domain ● i18n Support › production mode › should generate AMP pages with all locales ● i18n Support › production mode › should work with AMP first page with all locales ● i18n Support › production mode › should generate fallbacks with all locales ● i18n Support › production mode › should generate auto-export page with all locales ● i18n Support › production mode › should generate non-dynamic GSP page with all locales ● i18n Support › production mode › should not output GSP pages that returned notFound ● i18n Support › production mode › should 404 for GSP pages that returned notFound ● i18n Support › production mode › should transition on client properly for page that starts with locale ● i18n Support › production mode › should 404 for GSP that returned notFound on client-transition ● i18n Support › production mode › should render 404 for fallback page that returned 404 on client transition ● i18n Support › production mode › should render 404 for fallback page that returned 404 ● i18n Support › production mode › should render 404 for blocking fallback page that returned 404 on client transition ● i18n Support › production mode › should render 404 for blocking fallback page that returned 404 ● i18n Support › production mode › should not remove locale prefix for default locale ● i18n Support › production mode › should load getStaticProps page correctly SSR (default locale no prefix) ● i18n Support › production mode › should load getStaticProps fallback prerender page correctly SSR (default locale no prefix) ● i18n Support › production mode › should load getStaticProps fallback non-prerender page correctly (default locale no prefix ● i18n Support › production mode › should redirect to locale prefixed route for / ● i18n Support › production mode › should use default locale for / without accept-language ● i18n Support › production mode › should load getStaticProps page correctly SSR ● i18n Support › production mode › should load getStaticProps fallback prerender page correctly SSR ● i18n Support › production mode › should load getStaticProps fallback non-prerender page correctly ● i18n Support › production mode › should load getServerSideProps page correctly SSR (default locale no prefix) ● i18n Support › production mode › should navigate client side for default locale with no prefix ● i18n Support › production mode › should load getStaticProps fallback non-prerender page another locale correctly ● i18n Support › production mode › should load getStaticProps non-fallback correctly ● i18n Support › production mode › should load getStaticProps non-fallback correctly another locale ● i18n Support › production mode › should load getStaticProps non-fallback correctly another locale via cookie ● i18n Support › production mode › should load getServerSideProps page correctly SSR ● i18n Support › production mode › should load dynamic getServerSideProps page correctly SSR ● i18n Support › production mode › should navigate to another page and back correctly with locale ● i18n Support › production mode › should navigate to getStaticProps page and back correctly with locale ● i18n Support › production mode › should have pre-rendered /500 correctly Read more about building and testing Next.js in contributing.md.
Expand output● clientTraceMetadata › app router › should inject propagation data for a dynamically server-side-rendered page ● clientTraceMetadata › app router › hard loading a dynamic page twice should yield different dynamic trace data ● clientTraceMetadata › app router › should only insert the client trace metadata once ● clientTraceMetadata › app router › next dev only › should inject propagation data for a statically server-side-rendered page ● clientTraceMetadata › app router › next dev only › soft navigating to a dynamic page should not transform previous propagation data ● clientTraceMetadata › app router › next dev only › soft navigating to a static page should not transform previous propagation data ● clientTraceMetadata › pages router › should inject propagation data for a dynamically server-side-rendered page ● clientTraceMetadata › pages router › hard loading a dynamic page twice should yield different dynamic trace data ● clientTraceMetadata › pages router › next dev only › should inject propagation data for a statically server-side-rendered page ● clientTraceMetadata › pages router › next dev only › soft navigating to a dynamic page should not transform previous propagation data ● clientTraceMetadata › pages router › next dev only › soft navigating to a static page should not transform previous propagation data Read more about building and testing Next.js in contributing.md.
Expand output● with isrFlushToDisk config › dev support w/o next.config.js › should use cache and stale-while-revalidate when query is the same for internal image ● with isrFlushToDisk config › dev support w/o next.config.js › should use cached image file when parameters are the same for animated gif ● with isrFlushToDisk config › dev support w/o next.config.js › should maintain bmp ● with isrFlushToDisk config › dev support with next.config.js › should use cache and stale-while-revalidate when query is the same for external image ● with isrFlushToDisk config › dev support with next.config.js › should use cache and stale-while-revalidate when query is the same for internal image ● with isrFlushToDisk config › dev support with next.config.js › should use cached image file when parameters are the same for animated gif ● with isrFlushToDisk config › dev support with next.config.js › should maintain bmp ● with isrFlushToDisk config › dev support with next.config.js › should handle concurrent requests Read more about building and testing Next.js in contributing.md. |
isBatak
changed the title
disableWriteToCacheDir option to prevent writing optimized images to cacheisrFlushToDisk option to prevent writing optimized images to cache
isBatak
changed the title
isrFlushToDisk option to prevent writing optimized images to cacheisrFlushToDisk option to prevent writing optimized images to cache
vignesh1507
reviewed
Oct 17, 2024
styfle
reviewed
Nov 15, 2024
| @@ -903,14 +906,21 @@ export function runTests(ctx: RunTestsCtx) { | |||
| const etagOne = one.res.headers.get('etag') | |||
|
|
|||
| let json1 | |||
| await check(async () => { | |||
|
|
|||
| if (!ctx.isrFlushToDisk) { | |||
| json1 = await fsToJson(ctx.imagesDir) | |||
There was a problem hiding this comment.
Suggested change
| json1 = await fsToJson(ctx.imagesDir) | |
| json1 = await fsToJson(ctx.imagesDir) | |
| expect(json1).toStrictEqual({}) |
styfle
reviewed
Nov 15, 2024
Comment on lines
+938
to
+943
|
|
||
| if (!ctx.isrFlushToDisk) { | ||
| expect(json2).toStrictEqual({}) | ||
| } else { | ||
| expect(json2).toStrictEqual(json1) | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (!ctx.isrFlushToDisk) { | |
| expect(json2).toStrictEqual({}) | |
| } else { | |
| expect(json2).toStrictEqual(json1) | |
| } | |
| expect(json2).toStrictEqual(json1) |
styfle
reviewed
Nov 15, 2024
| @@ -5,5 +5,5 @@ const appDir = join(__dirname, '../app') | |||
| const imagesDir = join(appDir, '.next', 'cache', 'images') | |||
|
|
|||
| describe('with latest sharp', () => { | |||
| setupTests({ appDir, imagesDir }) | |||
| setupTests({ appDir, imagesDir, isrFlushToDisk: false }) | |||
There was a problem hiding this comment.
We need to keep one of these with isrFlushToDisk: true or else we won't be running all the tests
Suggested change
| setupTests({ appDir, imagesDir, isrFlushToDisk: false }) | |
| setupTests({ appDir, imagesDir, isrFlushToDisk: true }) |
styfle
reviewed
Nov 15, 2024
| @@ -22,5 +22,6 @@ describe('with minimumCacheTTL of 5 sec', () => { | |||
| }, | |||
| appDir, | |||
| imagesDir, | |||
| isrFlushToDisk: false, | |||
There was a problem hiding this comment.
Suggested change
| isrFlushToDisk: false, | |
| isrFlushToDisk: true, |
This one seems important to flush to disk so we run the corresponding tests
styfle
reviewed
Nov 15, 2024
| const appDir = join(__dirname, '../app') | ||
| const imagesDir = join(appDir, '.next', 'cache', 'images') | ||
|
|
||
| describe('with isrFlushToDisk config', () => { |
There was a problem hiding this comment.
Suggested change
| describe('with isrFlushToDisk config', () => { | |
| describe('with isrFlushToDisk: false config', () => { |
Let's update the existing tests to remove this new property and so that this test is the only one that uses false
styfle
reviewed
Nov 15, 2024
| @@ -974,15 +996,21 @@ export function runTests(ctx: RunTestsCtx) { | |||
| expect(five.res.headers.get('Content-Disposition')).toBe( | |||
| `${contentDispositionType}; filename="slow.webp"` | |||
| ) | |||
| await check(async () => { | |||
| if (!ctx.isrFlushToDisk) { | |||
There was a problem hiding this comment.
Let's update all these checks in util.ts to be more explicit:
if (ctx.isrFlushToDisk === false)
That way we don't consider undefined and false the same because we know the default behavior of undefined is actually true.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #25846: next/image option to disable fs cache
Description
This PR implements the use of the existing experimental configuration option, isrFlushToDisk, for Next.js, which prevents optimized images from being written to the cache directory. This feature can be useful in scenarios where disk writes need to be avoided, such as in environments with limited disk space or read-only file systems.
By enabling this option, the
x-nextjs-cacheresponse header will always returnMISSfor every request, indicating that no caching is taking place for optimized images.Configuration Example
To enable this feature, add the following to your
next.config.js: