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

micro fix of the cache limit check #60249

Merged

Conversation

vordgi
Copy link
Contributor

@vordgi vordgi commented Jan 5, 2024

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.

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 from previous PR 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.

@ijjk
Copy link
Member

ijjk commented Jan 5, 2024

Allow CI Workflow Run

  • approve CI run for commit: 5d151dd

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

2 similar comments
@ijjk
Copy link
Member

ijjk commented Jan 5, 2024

Allow CI Workflow Run

  • approve CI run for commit: 5d151dd

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

@ijjk
Copy link
Member

ijjk commented Jan 5, 2024

Allow CI Workflow Run

  • approve CI run for commit: 5d151dd

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

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.

Looks like we don't have a test for this error in dev so if you'd be interested in adding that in a follow-up that'd be great

@ijjk ijjk enabled auto-merge (squash) January 6, 2024 01:38
@ijjk
Copy link
Member

ijjk commented Jan 6, 2024

Stats from current PR

Default Build
General
vercel/next.js canary vordgi/next.js fix/incremental-cache-limit-for-custom-handler-2 Change
buildDuration 13s 13s N/A
buildDurationCached 7.1s 6.2s N/A
nodeModulesSize 200 MB 200 MB N/A
nextStartRea..uration (ms) 425ms 423ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vordgi/next.js fix/incremental-cache-limit-for-custom-handler-2 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.6 kB 28.6 kB N/A
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 240 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-2 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-2 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-2 Change
_buildManifest.js gzip 483 B 483 B
Overall change 483 B 483 B
Rendered Page Sizes
vercel/next.js canary vordgi/next.js fix/incremental-cache-limit-for-custom-handler-2 Change
index.html gzip 528 B 527 B N/A
link.html gzip 538 B 541 B N/A
withRouter.html gzip 521 B 522 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary vordgi/next.js fix/incremental-cache-limit-for-custom-handler-2 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-2 Change
middleware-b..fest.js gzip 622 B 623 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-2 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
Overall change 932 kB 932 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: 162b3ed

@ijjk ijjk merged commit 826955b into vercel:canary Jan 6, 2024
63 of 68 checks passed
@vordgi vordgi deleted the fix/incremental-cache-limit-for-custom-handler-2 branch January 6, 2024 05:32
agustints pushed a commit to agustints/next.js that referenced this pull request Jan 6, 2024
### 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>
ijjk added a commit that referenced this pull request Jan 9, 2024
### What?
In the previous PR (#60249), it was found that the case when the
response size is larger than 2MB is not sufficiently covered by tests
[[comment](#60249 (review))]

### How?
I added a few more checks.

Just checking the number of API calls is not enough - I believe it is
important to additionally verify what exactly the page received.

If I'm honest, I couldn't find the exact reason why data is loaded after
application start when using `customCacheHandler`, but without it -
during build. It seems like something in the butcher. Therefore, in this
part, I simply configured it for the current version.

---------

Co-authored-by: JJ Kasper <jj@jjsweb.site>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2024
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.

None yet

2 participants