Disable 2mb limit for custom incrementalCacheHandler #59976

vordgi · 2023-12-27T19:55:50Z

Fixing a bug

What?

Disable 2MB limit for custom incrementalCacheHandler

Why?

The limit is necessary because FetchCache has a 2MB limit, but it seems there was a miscommunication regarding the key coincidence, where fetchCache is a flag indicating that the method is called from fetch, rather than indicating that the FetchCache Provider is currently being used.

We do not use Vercel, and as I understand it, we do not have the opportunity to use this functionality.

In any case, it is more important for us to increase the limits, and in some cases, using a file storage is even preferable.

How?

I have created a flag that determines whether the use of FetchCache is possible at least in theory - if no custom provider is passed, and additionally configured it so that it is not an implementation of FetchCache as a protection against special individuals (like me :)).

If everything is fine, I will write proper tests.

Also, I would like to recommend making FileSystemCache public (i.e. support it as public functionality) so that it can be imported and extended or simply used to fix only it.

Fixes #48324 (partially)

ijjk · 2023-12-27T19:56:13Z

Allow CI Workflow Run

approve CI run for commit: 534d994

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

ijjk · 2023-12-27T19:56:13Z

Allow CI Workflow Run

approve CI run for commit: 7995883

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

packages/next/src/server/lib/incremental-cache/index.ts

test/e2e/fetch-cache-limit/fetch-cache-limit.test.ts

packages/next/src/server/lib/incremental-cache/index.ts

…dler

ijjk · 2024-01-04T22:25:47Z

Tests Passed

ijjk · 2024-01-04T22:27:53Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	vordgi/next.js fix/incremental-cache-limit-for-custom-handler	Change
buildDuration	13.7s	13.9s	⚠️ +135ms
buildDurationCached	7.8s	6.8s	N/A
nodeModulesSize	200 MB	200 MB	⚠️ +2.2 kB
nextStartRea..uration (ms)	436ms	447ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	vordgi/next.js fix/incremental-cache-limit-for-custom-handler	Change
193.HASH.js gzip	181 B	182 B	N/A
3f784ff6-HASH.js gzip	53.3 kB	53.3 kB	✓
433-HASH.js gzip	28.5 kB	28.6 kB	N/A
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	239 B	242 B	N/A
main-HASH.js gzip	31.7 kB	31.8 kB	N/A
webpack-HASH.js gzip	1.7 kB	1.7 kB	N/A
Overall change	98.5 kB	98.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vordgi/next.js fix/incremental-cache-limit-for-custom-handler	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	vordgi/next.js fix/incremental-cache-limit-for-custom-handler	Change
_app-HASH.js gzip	194 B	195 B	N/A
_error-HASH.js gzip	183 B	181 B	N/A
amp-HASH.js gzip	504 B	502 B	N/A
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.5 kB	2.5 kB	N/A
edge-ssr-HASH.js gzip	255 B	253 B	N/A
head-HASH.js gzip	350 B	349 B	N/A
hooks-HASH.js gzip	369 B	369 B	✓
image-HASH.js gzip	4.28 kB	4.28 kB	N/A
index-HASH.js gzip	255 B	256 B	N/A
link-HASH.js gzip	2.61 kB	2.61 kB	✓
routerDirect..HASH.js gzip	312 B	311 B	N/A
script-HASH.js gzip	385 B	383 B	N/A
withRouter-HASH.js gzip	307 B	308 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	3.4 kB	3.4 kB	✓

Client Build Manifests

	vercel/next.js canary	vordgi/next.js fix/incremental-cache-limit-for-custom-handler	Change
_buildManifest.js gzip	483 B	484 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	vordgi/next.js fix/incremental-cache-limit-for-custom-handler	Change
index.html gzip	526 B	526 B	✓
link.html gzip	539 B	539 B	✓
withRouter.html gzip	523 B	521 B	N/A
Overall change	1.06 kB	1.06 kB	✓

Edge SSR bundle Size

	vercel/next.js canary	vordgi/next.js fix/incremental-cache-limit-for-custom-handler	Change
edge-ssr.js gzip	93.8 kB	93.8 kB	N/A
page.js gzip	147 kB	147 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	vordgi/next.js fix/incremental-cache-limit-for-custom-handler	Change
middleware-b..fest.js gzip	625 B	624 B	N/A
middleware-r..fest.js gzip	151 B	151 B	✓
middleware.js gzip	37.4 kB	37.4 kB	N/A
edge-runtime..pack.js gzip	1.92 kB	1.92 kB	✓
Overall change	2.07 kB	2.07 kB	✓

Next Runtimes

	vercel/next.js canary	vordgi/next.js fix/incremental-cache-limit-for-custom-handler	Change
app-page-exp...dev.js gzip	169 kB	169 kB	✓
app-page-exp..prod.js gzip	94.2 kB	94.2 kB	✓
app-page-tur..prod.js gzip	94.9 kB	94.9 kB	✓
app-page-tur..prod.js gzip	89.5 kB	89.5 kB	✓
app-page.run...dev.js gzip	139 kB	139 kB	✓
app-page.run..prod.js gzip	88.8 kB	88.8 kB	✓
app-route-ex...dev.js gzip	24.1 kB	24.1 kB	✓
app-route-ex..prod.js gzip	16.7 kB	16.7 kB	✓
app-route-tu..prod.js gzip	16.7 kB	16.7 kB	✓
app-route-tu..prod.js gzip	16.3 kB	16.3 kB	✓
app-route.ru...dev.js gzip	23.5 kB	23.5 kB	✓
app-route.ru..prod.js gzip	16.3 kB	16.3 kB	✓
pages-api-tu..prod.js gzip	9.38 kB	9.38 kB	✓
pages-api.ru...dev.js gzip	9.65 kB	9.65 kB	✓
pages-api.ru..prod.js gzip	9.37 kB	9.37 kB	✓
pages-turbo...prod.js gzip	21.9 kB	21.9 kB	✓
pages.runtim...dev.js gzip	22.6 kB	22.6 kB	✓
pages.runtim..prod.js gzip	21.9 kB	21.9 kB	✓
server.runti..prod.js gzip	49.5 kB	49.5 kB	N/A
Overall change	883 kB	883 kB	✓

Diff details

Diff for page.js

Diff too large to display

Diff for edge-ssr.js

Diff too large to display

Diff for server.runtime.prod.js

Diff too large to display

Commit: 7e028fb

vordgi · 2024-01-05T04:35:05Z

I don’t even know why I didn’t immediately test just fetch, it’s much better, thanks @ijjk 🧡

### Fixing a bug I'm sorry, I have no idea how I managed to delete this, as I just copied the code. However, I had to figure out why the tests passed. When I test the running application locally, it works as expected. So, when a route is first used in a running production application in a test, `ctx.fetchCache` is `undefined`. Therefore, the error rule does not trigger on the first request, and the cache is successfully written. This may result in artifacts in FetchCache in Vercel and possibly in other parts of the code. ```ts if ( ctx.fetchCache && // <- Undefined on first request // we don't show this error/warning when a custom cache handler is being used // as it might not have this limit !this.hasCustomCacheHandler && JSON.stringify(data).length > 2 * 1024 * 1024 ) { if (this.dev) { throw new Error(`fetch for over 2MB of data can not be cached`) } return } ``` I'm 95% sure that this is a bug and it shouldn't work like this. I'll look into this later and if there is an error, I'll put it into a task and try to figure it out later. Unfortunately, the [test](https://github.com/vordgi/next.js/blob/7e028fb6d8598cfcca8de514e4608374f931c973/test/e2e/app-dir/app-static/app-static.test.ts#L3080) from [previous PR](#59976) is not working yet, although it is correct from my point of view. I think it should be kept and the problem above should be fixed. Co-authored-by: JJ Kasper <jj@jjsweb.site>

### Fixing a bug ### What? Disable 2MB limit for custom incrementalCacheHandler ### Why? The limit is necessary because `FetchCache` has a 2MB limit, but it seems there was a miscommunication regarding the key coincidence, where `fetchCache` is a flag indicating that the method is called from fetch, rather than indicating that the `FetchCache` Provider is currently being used. We do not use Vercel, and as I understand it, we do not have the opportunity to use this functionality. In any case, it is more important for us to increase the limits, and in some cases, using a file storage is even preferable. ### How? I have created a flag that determines whether the use of `FetchCache` is possible at least in theory - if no custom provider is passed, and additionally configured it so that it is not an implementation of `FetchCache` as a protection against special individuals (*like me :)*). If everything is fine, I will write proper tests. Also, I would like to recommend making `FileSystemCache` public (_i.e. support it as public functionality_) so that it can be imported and extended or simply used to fix only it. Fixes vercel#48324 (partially) --------- Co-authored-by: JJ Kasper <jj@jjsweb.site>

### Fixing a bug I'm sorry, I have no idea how I managed to delete this, as I just copied the code. However, I had to figure out why the tests passed. When I test the running application locally, it works as expected. So, when a route is first used in a running production application in a test, `ctx.fetchCache` is `undefined`. Therefore, the error rule does not trigger on the first request, and the cache is successfully written. This may result in artifacts in FetchCache in Vercel and possibly in other parts of the code. ```ts if ( ctx.fetchCache && // <- Undefined on first request // we don't show this error/warning when a custom cache handler is being used // as it might not have this limit !this.hasCustomCacheHandler && JSON.stringify(data).length > 2 * 1024 * 1024 ) { if (this.dev) { throw new Error(`fetch for over 2MB of data can not be cached`) } return } ``` I'm 95% sure that this is a bug and it shouldn't work like this. I'll look into this later and if there is an error, I'll put it into a task and try to figure it out later. Unfortunately, the [test](https://github.com/vordgi/next.js/blob/7e028fb6d8598cfcca8de514e4608374f931c973/test/e2e/app-dir/app-static/app-static.test.ts#L3080) from [previous PR](vercel#59976) is not working yet, although it is correct from my point of view. I think it should be kept and the problem above should be fixed. Co-authored-by: JJ Kasper <jj@jjsweb.site>

disable 2mb limit for custom incrementalcachehandler

534d994

vordgi requested review from timneutkens, ijjk, shuding, huozhi, feedthejim, ztanner and wyattjoh as code owners December 27, 2023 19:55

ijjk added area: tests type: next labels Dec 27, 2023

ijjk reviewed Jan 3, 2024

View reviewed changes

packages/next/src/server/lib/incremental-cache/index.ts Outdated Show resolved Hide resolved

ijjk reviewed Jan 3, 2024

View reviewed changes

test/e2e/fetch-cache-limit/fetch-cache-limit.test.ts Outdated Show resolved Hide resolved

move fetch-cache-limit test to app-static test

2b8daf8

ijjk reviewed Jan 3, 2024

View reviewed changes

packages/next/src/server/lib/incremental-cache/index.ts Outdated Show resolved Hide resolved

vordgi and others added 3 commits January 4, 2024 07:52

renaming custom cache handler detection flag

66886ee

Merge branch 'canary' into fix/incremental-cache-limit-for-custom-han…

7995883

…dler

Merge branch 'canary' into fix/incremental-cache-limit-for-custom-han…

0d598e2

…dler

update tests

7e028fb

ijjk approved these changes Jan 4, 2024

View reviewed changes

ijjk merged commit 269020a into vercel:canary Jan 4, 2024
65 of 68 checks passed

vordgi deleted the fix/incremental-cache-limit-for-custom-handler branch January 5, 2024 04:35

vordgi mentioned this pull request Jan 5, 2024

micro fix of the cache limit check #60249

Merged

samcx mentioned this pull request Jan 6, 2024

chore: update turbo to the latest #60293

Closed

github-actions bot added the locked label Jan 19, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable 2mb limit for custom incrementalCacheHandler #59976

Disable 2mb limit for custom incrementalCacheHandler #59976

vordgi commented Dec 27, 2023

ijjk commented Dec 27, 2023

ijjk commented Dec 27, 2023 •

edited

Loading

ijjk commented Jan 4, 2024 •

edited

Loading

ijjk commented Jan 4, 2024 •

edited

Loading

vordgi commented Jan 5, 2024

Disable 2mb limit for custom incrementalCacheHandler #59976

Disable 2mb limit for custom incrementalCacheHandler #59976

Conversation

vordgi commented Dec 27, 2023

Fixing a bug

What?

Why?

How?

ijjk commented Dec 27, 2023

ijjk commented Dec 27, 2023 • edited Loading

ijjk commented Jan 4, 2024 • edited Loading

Tests Passed

ijjk commented Jan 4, 2024 • edited Loading

Stats from current PR

vordgi commented Jan 5, 2024

ijjk commented Dec 27, 2023 •

edited

Loading

ijjk commented Jan 4, 2024 •

edited

Loading

ijjk commented Jan 4, 2024 •

edited

Loading