Skip to content

fix: support non-prerendered dynamic routes with fallback false #1541

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

Merged
merged 29 commits into from
Sep 9, 2022

Conversation

orinokai
Copy link
Contributor

@orinokai orinokai commented Aug 16, 2022

Summary

Next.js uses an internal cache to persist page creation and updates during incremental static regeneration (ISR).

In order to preserve atomic deploys, Netlify has a different model that uses our ODB functions for creating and updating pages after deployment.

To support this, we currently patch the Next.js server to disable the internal cache, which ensures our ODB functions always receive fresh content and can handle the caching and revalidation directly.

However, our current solution has an incidental bug that bypasses the fallback: false and fallback: true functionality and always serves the page like fallback: blocking. In addition, it creates an unnecessary function invocation and runs getStaticProps unexpectedly.

Fixing this bug enables support for all the fallback options, but doesn’t work in one specific case: pre-rendered ISR pages with fallback: false and a revalidation TTL. These pages get served as a 404 because there is no fallback and the server thinks there is no statically pre-rendered page. This does not happen for ISR pages without revalidation because they are served statically, bypassing the origin entirely.

This PR implements a different strategy: we continue to disable cache writes, but enable cache reads and force manual revalidation mode so fresh content is always served.

Further work is currently ongoing in order to support bypassing the ODB handler for 404 pages.

  • enable cache reads to support fallback: false and fallback: true, but force manual revalidation for fresh content
  • add tests

Test plan

See test coverage

Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal

Fennec fox stack

Fixes #1179


🧪 Once merged, make sure to update the version if needed and that it was published correctly.

@orinokai orinokai self-assigned this Aug 16, 2022
@netlify
Copy link

netlify bot commented Aug 16, 2022

Deploy Preview for netlify-plugin-nextjs-demo ready!

Name Link
🔨 Latest commit 55de50a
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-nextjs-demo/deploys/631b539851bd0b00098bd53c
😎 Deploy Preview https://deploy-preview-1541--netlify-plugin-nextjs-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 16, 2022

Deploy Preview for next-hp-edge-demo ready!

Name Link
🔨 Latest commit 55de50a
🔍 Latest deploy log https://app.netlify.com/sites/next-hp-edge-demo/deploys/631b53987bd35600080dc592
😎 Deploy Preview https://deploy-preview-1541--next-hp-edge-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 16, 2022

Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo failed.

Name Link
🔨 Latest commit 55de50a
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-nextjs-nx-monorepo-demo/deploys/631b53978cacba00089bbc2b

@github-actions github-actions bot added the type: bug code to address defects in shipped code label Aug 16, 2022
@netlify
Copy link

netlify bot commented Aug 16, 2022

Deploy Preview for netlify-plugin-nextjs-export-demo ready!

Name Link
🔨 Latest commit 55de50a
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-nextjs-export-demo/deploys/631b53987946e300088503f6
😎 Deploy Preview https://deploy-preview-1541--netlify-plugin-nextjs-export-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 16, 2022

Deploy Preview for next-plugin-canary ready!

Name Link
🔨 Latest commit 55de50a
🔍 Latest deploy log https://app.netlify.com/sites/next-plugin-canary/deploys/631b5398b6a31d00089d002c
😎 Deploy Preview https://deploy-preview-1541--next-plugin-canary.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 16, 2022

Deploy Preview for next-plugin-edge-middleware ready!

Name Link
🔨 Latest commit 55de50a
🔍 Latest deploy log https://app.netlify.com/sites/next-plugin-edge-middleware/deploys/631b539875cbc50008e7d332
😎 Deploy Preview https://deploy-preview-1541--next-plugin-edge-middleware.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 16, 2022

Deploy Preview for netlify-plugin-nextjs-next-auth-demo ready!

Name Link
🔨 Latest commit 55de50a
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-nextjs-next-auth-demo/deploys/631b539843978f000a924fd4
😎 Deploy Preview https://deploy-preview-1541--netlify-plugin-nextjs-next-auth-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 16, 2022

Deploy Preview for next-i18next-demo ready!

Name Link
🔨 Latest commit 55de50a
🔍 Latest deploy log https://app.netlify.com/sites/next-i18next-demo/deploys/631b5398d0f4fd000836eab2
😎 Deploy Preview https://deploy-preview-1541--next-i18next-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 16, 2022

Deploy Preview for nextjs-plugin-custom-routes-demo ready!

Name Link
🔨 Latest commit 55de50a
🔍 Latest deploy log https://app.netlify.com/sites/nextjs-plugin-custom-routes-demo/deploys/631b539814b301000a927f25
😎 Deploy Preview https://deploy-preview-1541--nextjs-plugin-custom-routes-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 16, 2022

Deploy Preview for netlify-plugin-nextjs-static-root-demo ready!

Name Link
🔨 Latest commit 55de50a
🔍 Latest deploy log https://app.netlify.com/sites/netlify-plugin-nextjs-static-root-demo/deploys/631b5397e3c11300081349fe
😎 Deploy Preview https://deploy-preview-1541--netlify-plugin-nextjs-static-root-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Aug 16, 2022

Deploy Preview for next-plugin-rsc-demo failed.

Name Link
🔨 Latest commit 55de50a
🔍 Latest deploy log https://app.netlify.com/sites/next-plugin-rsc-demo/deploys/631b53989d4e7e00087bbcee

@hu0p
Copy link

hu0p commented Aug 23, 2022

adding documentation to warn i18n users who don't prerender all their locales

Is the motivation for this due to Matt's comment or is this a nice to have? I'm not sure this is necessary (unless I'm missing something) as it appears it's the intended behavior according to the docs (excerpt below).

For pages using getStaticProps with Dynamic Routes, all locale variants of the page desired to be prerendered need to be returned from getStaticPaths. Along with the params object returned for paths, you can also return a locale field specifying which locale you want to render. For example:

@orinokai orinokai force-pushed the rs/fallback-false-fix branch from a5b8971 to 8a361ae Compare September 2, 2022 12:59
@orinokai
Copy link
Contributor Author

orinokai commented Sep 2, 2022

Thanks @hu0p, that last item on the checklist is just because the current behaviour will render all locales regardless of whether a user specifies them in getStaticPaths. The new behaviour will 404 for unspecified locales, which is the correct behaviour, as you pointed out, but probably needs to be called out in the release notes.

@orinokai orinokai changed the title fix: bypass handler function for non-prerendered dynamic routes without fallback fix: force manual revalidate to support non-prerendered dynamic routes without fallback Sep 2, 2022
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't totally understand what's happening with the SSG cache. What happens when it tries to write to disk? Does it handle the failure? Is this being done so that it can return the prebuilt placeholder HTML?

@hu0p
Copy link

hu0p commented Sep 2, 2022

Thanks @hu0p, that last item on the checklist is just because the current behaviour will render all locales regardless of whether a user specifies them in getStaticPaths. The new behaviour will 404 for unspecified locales, which is the correct behaviour, as you pointed out, but probably needs to be called out in the release notes.

Got it. Thanks for the clarification! Watching this as much as I can. I appreciate both of your work on all of this. Thank you! I've been eying that highlighted portion of the redirects file off and on for months. When this lands it'll be a huge resolution for us.

@orinokai orinokai changed the title fix: force manual revalidate to support non-prerendered dynamic routes without fallback fix: support non-prerendered dynamic routes without fallback Sep 5, 2022
@orinokai
Copy link
Contributor Author

orinokai commented Sep 5, 2022

@ascorbic without an ssgCacheKey, ResponseCache.get() does not read the cache and instead runs the responseGenerator callback, which either handles fallback: true, fallback: false or generates fresh page content with doRender; however, the callback will skip fallback handling if ssgCacheKey is not set, which is what is currently happening. Reinstating ssgCacheKey and instead forcing isManualRevalidate = true still has the same effect within ResponseCache.get(), but lets the responseGenerator callback fall through to the fallback handling.

I will switch to the header method, which will remove the need for the 1st patch, and I may be able to avoid the 2nd patch too with the revalidateOnlyGenerated option, but will need testing. The 3rd patch is necessary, afaik, because otherwise Next.js doesn't send SWR cache control headers on 404 pages which means we can't handle ISR 404 pages with our ODBs (whereas Vercel handles them with the internal cache).

@@ -1,7 +1,124 @@
describe('Static Routing', () => {
it('loads show #42 via SSR on a static route', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to include the show numbers here. Just "the correct page" or something

@orinokai orinokai changed the title fix: support non-prerendered dynamic routes without fallback fix: support non-prerendered dynamic routes with fallback false Sep 6, 2022
@orinokai orinokai marked this pull request as ready for review September 7, 2022 16:14
@orinokai orinokai requested a review from a team September 7, 2022 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge type: bug code to address defects in shipped code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Fallback false is ignored
4 participants