Docs: Updating guides on PPR and ISR #81307
Conversation
Updating the docs - the code for jsx and tsx versions should use the switcher
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
|
Hey @icyJoseph. I see the jobs failing, could you please have a look if everything is okay? Is it only due to the "1 approving review"? Thanks :) |
|
@MichalMoravik we have to manually approve CI jobs to external contributor PRs for security reasons. For a docs-only change though, CI isn't that important (most stuff gets skipped anyways). |
| return { paths, fallback: false } | ||
| // If a request comes in for a path that hasn't been generated (e.g. /blog/25), | ||
| // Next.js will server-render the page on-demand. | ||
| return { paths, fallback: 'blocking' } // setting to false will 404 on unknown paths instead |
There was a problem hiding this comment.
Thanks for this one. It looks like when we merged the documents, we accidentally set this to false.
| // Next.js will invalidate the cache when a | ||
| // request comes in, at most once every 60 seconds. | ||
| export const revalidate = 60 | ||
|
|
||
| // We'll prerender only the params from `generateStaticParams` at build time. | ||
| // If a request comes in for a path that hasn't been generated, | ||
| // Next.js will server-render the page on-demand. | ||
| export const dynamicParams = true // or false, to 404 on unknown paths | ||
| export const dynamicParams = true // setting to false will 404 on unknown paths instead |
There was a problem hiding this comment.
Since we are trying to improve the docs here, I am a bit torn.
true is the default value - maybe it is okay to leave it as is
// We'll prerender only the params from `generateStaticParams` at build time.
// Using `dynamicParams` set to true (default value) If a request comes in
// for a path that was not generated at build time
// Next.js will attempt to server-render the page on-demand.
// Use `dynamicParams` set to false to response with 404 instead.
export const dynamicParams = trueThat being said, this feels like information that perhaps shouldn't be in a comment, but appended to the 6th point in the "Here's how this example works:" list.
There was a problem hiding this comment.
@icyJoseph I totally agree. It makes much more sense, I removed it from the code example and added the note about 404 to the 6th point in the description.
Do you think we should remove this entire block of comments and dynamicParams set to the default value altogether? I'm thinking about that. We can just get a rid of it and then make a link in the 6th point to the docs of dynamicParams. What would you say?
There was a problem hiding this comment.
I think, from reading feedback from people, some repetition is ok, and point 6 should link to dynamicParams yes
|
Thanks for the PR - I've left some thoughts, but all in all I think we need some of these changes |
…ing from the code example
|
Let's not forget to merge canary and resolve the conflict too - once again thanks for the PR |
…amlining the description underneath the example
|
@icyJoseph Hey! Since Also, I could not add |
|
@icyJoseph sorry to ping you like this, but do you think those changes are good now? |
|
Hey guys, do you think we could merge this? It's been one month since I made this PR. @icyJoseph @bgw @ijjk |
|
Hi, - let me take a look again, I had been OoO - just picking up work again. |
|
Ah, sorry for pinging you then, my bad. And thanks a lot for approval. btw it's protected, I won't be able to merge. |
The PR fixes issues in two different guides.
PPR
In PPR, the code for jsx and tsx versions should use the language switcher. Currently, these are displayed below each other in the guide which seems unintentional.
ISR
This change fixes multiple issues:
fixes in the example:
the example currently does not match the text below (Here's how this example works:). It says:
that is of course not correct because there are only 25 blog posts returned from the API. No blog post data is returned for
/blog/26, making this example wrong and confusing.By slicing the posts and supplying only 24 of 25 blog posts for static generation during the build process, the guide becomes much more explicit - the 25th blog post can be used for inspecting the workings of the on-the-fly generation and caching. Turning
dynamicParamstofalseortrue, one can now truly test this behaviour with the 25th blog.Second, the same example in Pages Router docs includes:
In this case, again, this part below:
is incorrect. When setting
fallbacktofalse, the page is never generated on-demand and returns 404 instead as there is no 26th blog post in the fetched data. So the fix changes the value offallbacktoblocking, to mirror the App Router'sdynamicParamsvalue in the guide. This makes is more consistent and, more importantly, reproducible example.clarifying the on-demand section:
The sections dedicated to on-demand revalidation do not currently mention that revalidation works differently from the time-based one. One may assume the first request after on-demand revalidation returns stale data (just like time-based).
But that's not true, the next request after on-demand revalidation (with Server Actions for example) generates the content on-the-fly and returns fresh data right away. It wasn't clear to me so I tested it myself and was surprised (and happy) that it is this smart. I think it's very important to clarify this distinction in the guide.
I hope this makes sense. I'm quite responsive these days and will change it based on your feedback (if any). Thanks!