fix: new ISR cache handling to resolve regression in 13.4 #2165
Conversation
✅ Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-static-root-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-export-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for netlify-plugin-nextjs-next-auth-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for nextjs-plugin-custom-routes-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-i18next-demo ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-plugin-edge-middleware ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for next-plugin-canary ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Got all the tests passing after several reruns except for two which are definitely related to the changes. Looking into these valid failures. |
| @@ -386,82 +336,6 @@ export const getDependenciesOfFile = async (file: string) => { | |||
| return dependencies.files.map((dep) => resolve(dirname(file), dep)) | |||
| } | |||
|
|
|||
| const baseServerReplacements: Array<[string, string]> = [ | |||
There was a problem hiding this comment.
We no longer need the server patches her because we handle this in NetlifyNextServer now.
| @@ -69,9 +69,6 @@ const makeHandler = ({ conf, app, pageRoot, NextServer, staticManifest = [], mod | |||
|
|
|||
| // We don't want to write ISR files to disk in the lambda environment | |||
| conf.experimental.isrFlushToDisk = false | |||
| // This is our flag that we use when patching the source | |||
| // eslint-disable-next-line no-underscore-dangle | |||
| process.env._REVALIDATE_SSG = 'true' | |||
There was a problem hiding this comment.
This was used for the server patches so is no longer required.
| @@ -189,28 +187,6 @@ describe('files utility functions', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| describeCwdTmpDir('file patching', () => { | |||
There was a problem hiding this comment.
Tests are obsolete as we no longer patch server files.
…c route with fallback: true
| @@ -66,8 +66,7 @@ describe('Dynamic Routing', () => { | |||
| expect(res.status).to.eq(200) | |||
| // expect 'odb' until https://github.com/netlify/pillar-runtime/issues/438 is fixed | |||
| expect(res.headers).to.have.property('x-nf-render-mode', 'odb') | |||
| // expect 'Bitten' until the above is fixed and we can test for fallback 'Loading...' message | |||
| expect(res.body).to.contain('Bitten') | |||
| expect(res.body).to.contain('Loading...') | |||
There was a problem hiding this comment.
This works now @orinokai. It was failing previously not finding Bitten which was expected pre the changes in this PR.
| // eslint-disable-next-line no-underscore-dangle | ||
| if (headers['x-nf-builder-cache'] === 'revalidate' && !headers.__prerender_bypass) { | ||
| console.log(`Revalidating ${url}`) | ||
| if (!headers.__prerender_bypass) { |
There was a problem hiding this comment.
Gah, I realized last night that if we only revalidate when an ODB sends that header, then middleware will still be enabled for all other requests (because we're now relying on revalidate mode to disable middleware at the origin). This commit reverts that, so we now revalidate on every request and will have to find a different way to serve the initial static content (but perhaps that's for a separate PR, like you said).
…d dynamic route with fallback: true" This reverts commit 14c8a35.
We are leaning on the E2E tests for now, which should be fine, but the next bit of 13.4 work will be fixing the on-demand revalidation and that includes tests for the server code in this PR. So I'll be updating those tests during that work. |
|
That's @orinokai. Great work from you @nickytonline! 🎉 |
|
Thanks for working on this! I'm curious, why was patching Next Server necessary in the first place? |
|
I think this resolves the regression as far back as next v13.2 |
hi @charkour, patching the Next server was previously necessary to disable the internal ISR cache so that we can handle caching on our edge CDN, but thankfully we can now achieve this by forcing on-demand revalidation, which is more reliable - and yep, 13.2 sounds right because that is when Next introduced their new caching strategy |
|
Roger, thanks for the explanation! That's helpful to know |
Description
Tests
Existing tests should run as before