-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
docs: react 18, streaming SSR, rsc with new apis #33986
Conversation
Apologies for an unsolicited review 😅 I was mainly curious in this PR as a consumer of docs, but could not resist from leaving a couple of comments. Great stuff – thanks a lot for your work on React 18 support! 🚀 |
@kachkaev Thanks for the nice feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, these new docs look great! Very thoughtfully done :)
Below are mostly stylistic/grammatical nitpicks, though there are a few places where I think more clarification and examples would be useful for folks with less context than you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments (realized I missed the RSC file!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making all the changes! A few more comments
Co-authored-by: Kara <kara@users.noreply.github.com>
We will need to reflect the changes made here, as well: #34068 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Co-authored-by: Kara <kara@users.noreply.github.com>
|
||
import Content from '../components/content' | ||
|
||
// These two ways are identical: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly identical, as React.lazy
doesn't do any preloading. Might be worth suggesting that you continue using next/dynamic
for now for performance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we enable { suspense: true }
it's using React.lazy
and doesn't do preloading I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see the issue -- we just haven't implemented the preloading yet (since it happens after the shell is flushed, where all of the existing preloading is done today), but we should. Otherwise there's no reason to have the { suspense: true }
option instead of just recommending that people use React.lazy
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The need for {suspense: true}
makes it feel second-class. Do we plan to flip the default at some point in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that currently that would be a breaking change without an incremental upgrade path: your application using next/dynamic
would break when upgrading to React 18 unless you add suspense boundaries first, but you can't add suspense boundaries until you upgrade to React 18. It also changes the semantics a bit, since pre-Suspense, the loading state is associated with the next/dynamic
component, rather than the parent where the component is rendered.
Changing the default in the future is a possibility, but it's likely a major version away. Some shorter term options might be:
- If we could detect that we're rendering inside a
<Suspense>
boundary, we could assume you want the suspense behavior - If we could somehow port the preloading behavior to
React.lazy
, we could push users toward that instead, when using React>=18
Both of these options would probably require some support in React though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It occurred to me that with facebook/react#23304 and facebook/react#23267 this might not actually be a breaking change anymore, if the transition semantics apply to root.render()
as well... Would need to investigate further though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what we could theoretically do is use suspense by default during SSR and initial hydration if you're using React>=18, since that behavior shouldn't change (after the next RC).
For client-side navigations that would still be at risk of suspending, we could synchronously render the legacy non-Suspense based fallback that you provide with next/dynamic
, and then also warn that you should add a suspense boundary and move your fallback there instead.
Let me think about it some more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that facebook/react#23304 does in fact change the behavior for root.render(...)
as long as it's not wrapped in flushSync
. So yeah, it would probably make sense to make this change.
Thanks for flagging @gaearon
|
||
#### Data Fetching | ||
|
||
Currently, data fetching within `Suspense` boundaries on the server side is not fully supported, which could lead to mismatching between server and client. In the short-term, please don't try data fetching within `Suspense`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be worth adding a caveat to the main React 18 doc about using any existing (i.e. pre-React 18) suspense libraries, especially if you're relying on a library like react-ssr-prepass
. This might not apply to many Next.js users though, unsure.
Co-authored-by: Lee Robinson <me@leerob.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks really great!
Wanted to make sure this is included in the next release, so merged - if there are any additional comments here unresolved or further questions, we can do a follow up 😄 Thank you! |
Documentation / Examples
Separate react 18 into 3 parts:
And and
next/streaming
as new APIyarn lint