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

Incremental Static Regeneration - docs update explaining surprising behavior #66151

Merged
merged 12 commits into from
Jun 10, 2024

Conversation

karlkeefer
Copy link
Contributor

The requirement to export a generateStaticParams to get static cache behavior after build time was really surprising behavior for me, and I think others:

Potentially this is a bug, and not something that should be fixed with documentation?
I don't understand next.js caching enough to make that determination, so instead I'm proposing these changes to docs which might be encountered by folks who are surprised by this cache behavior.

One point in favor of this being a bug: The CLI reports that a route is SSG enabled in the build output, but doesn't actually cache post-build page renders if this export is missing.

@awinogrodzki made a demo repo showing this behavior, as described here.

@karlkeefer karlkeefer requested review from a team as code owners May 24, 2024 00:44
@karlkeefer karlkeefer requested review from timeyoutakeit and leerob and removed request for a team May 24, 2024 00:44
@ijjk ijjk added the Documentation Related to Next.js' official documentation. label May 24, 2024
@ijjk
Copy link
Member

ijjk commented May 24, 2024

Allow CI Workflow Run

  • approve CI run for commit: c0505af

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

Copy link
Member

@samcx samcx left a comment

Choose a reason for hiding this comment

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

Thank you for submitting a PR!

:lgtm:.

One point in favor of this being a bug: The CLI reports that a route is SSG enabled in the build output, but doesn't actually cache post-build page renders if this export is missing.

Good callout! We need to I think we need to improve our CLI output here.

The requirement to export a generateStaticParams to get static cache behavior after build time was really surprising behavior for me, and I think others:

Hmm, maybe somewhat! The behavior is similar to Pages Router—if you only had a dynamic page, it would not opt you into ISR—you would have to use getStaticPaths. Similarly, you have to use generateStaticParams here.

A possible discussion to have is whether revalidate should just enable it, without an empty generateStaticParams. :frog-eyes:

@samcx samcx self-requested a review June 4, 2024 20:58
leerob
leerob previously requested changes Jun 5, 2024
Copy link
Member

@leerob leerob left a comment

Choose a reason for hiding this comment

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

Would love to get @delbaoliveira's opinion here before merging 🙏

@karlkeefer
Copy link
Contributor Author

karlkeefer commented Jun 5, 2024

@samcx I think my confusion here may stem from the fact that I didn't use pages router before.

As more folks start to use next-js after app router is the recommended default, the level of surprise among users may rise ;)

Thanks for taking a look at the PR 🙌

@samcx
Copy link
Member

samcx commented Jun 5, 2024

As more folks use next-js after app router is the recommended default, the level of surprise among users may rise ;)

@karlkeefer Makes sense and noted! We are working to improve the App Router's caching story which will most likely involve improving the Route Segment Config, so we could end up improving this to cause less confusion.

I'll defer to Delba for further review on this :pr: :blob_bowing:

@delbaoliveira
Copy link
Contributor

Thank you for raising this @karlkeefer, I've pushed a commit to rephrase a few things.

@samcx maybe not supplying generateStaticParams should be the equivalent of supplying an empty array. But I'll defer that to your team :)

@samcx samcx requested a review from leerob June 10, 2024 15:36
@samcx samcx enabled auto-merge (squash) June 10, 2024 15:42
@samcx samcx dismissed leerob’s stale review June 10, 2024 15:43

Delba was able to review this!

@samcx samcx merged commit 5be04cf into vercel:canary Jun 10, 2024
33 checks passed
@rijk
Copy link

rijk commented Jun 12, 2024

A possible discussion to have is whether revalidate should just enable it, without an empty generateStaticParams. :frog-eyes:

Or setting export const dynamic = 'force-static'. Having those options available but not actually making a route static, unless you add a generateStaticParams function that returns an empty array is the kind of DX that makes someone lose their mind.

@samcx
Copy link
Member

samcx commented Jun 14, 2024

@rijk Thanks for the feedback!

Actually not 100% with export const dynamic = 'force-static—taking a look :frog-eyes:

@samcx
Copy link
Member

samcx commented Jun 20, 2024

@rijk It turns out you can use force-static! I'll be updating our :nextjs-spin: :docs: again :blob_bowing:

samcx added a commit that referenced this pull request Jun 20, 2024
## Why?

You can also utilize `export cont dynamic = 'force-static'` to ISR pages
at runtime, instead of having to returning an empty array in
`generateStaticParams`.

x-ref: #66151,
#62195 (comment)
@github-actions github-actions bot added the locked label Jul 4, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation Related to Next.js' official documentation. locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants