feat(react-18): Add official React 18 SSR support #290
Conversation
Leftovers: The class component lifecycle shouldComponentUpdate has to be migrated to a functional component.
Leftovers: The media component currently supports only the render prop variant. Other required components to make this approach work, such as suspense wrapper and suspender, are not integrated. A discussion has to be started if those components have to live in the user-land.
| children, | ||
| }) => { | ||
| /** | ||
| * TODO: The class component lifecycle shouldComponentUpdate |
There was a problem hiding this comment.
Since there's no clear way to check state within React.memo just going to skip this for now.
| } | ||
|
|
||
| return ( | ||
| <Suspense fallback={null}> |
There was a problem hiding this comment.
don't we want to enable custom fallbacks here or it's not needed because of SSR?
There was a problem hiding this comment.
Whatever we'd want to catch will be caught higher up in their code (if the user decides to show a suspense boundary); this is just to trigger the cleanup mechanism internal to the lib
|
@damassi if you have time, could you please adjust the pr with the comments I made and provide a new release. Thx! |
|
@gurkerl83 - which comments are you referring to? (I'm not seeing anything in this PR.) Hoping to wrap this up today; we're in the middle of a product launch right now so keep getting pulled away and onto other tasks but should be able to get to this later. |
|
@damassi Not sure, but I see a few of mine in pending state. They are more syntactic wise not passing renderChildren and isPending, only cassName in case of render prop variant… |
|
If you can hit the 'finish review' button then i'll see your comments; i can't see them until they've been posted. |
|
Sry I don’t See such a Button on my end. As you are the committer I think you have to resolve. When it’s not possible simply merge, because everything is ok. I can provide something which resolves those things tomorrow. |
If i'm understanding correctly, it seems like you've submitted PR comments as part of a PR review, but haven't completed the review; if you open this UI you should be able to submit the review and I will be able to see your comments:
Other than that, not too sure. Maybe try re-adding your comments and posting them as single comments, per this button?
Forgive me if i'm misunderstanding! But i can't see your PR comments as it is, and thus can't address them. |
damassi
force-pushed
the
gurkerl83-React-18-Suspense-Integration
branch
from
c7da912
to
8bb8b84
Compare
|
PR is green and all of our existing tests pass, so going to ship this out @gurkerl83. For any other follow-up issues please comment on the PR here or in a new issue and I'll take care of it tomorrow. Thanks again so much for your help with this 🙏 💯 |
|
🚀 PR was released in |
Closes #289
Closes #260
All credit goes to @gurkerl83 and the work he spearheaded in #289 and the investigations in #260. Much appreciated!
This PR integrates that work and updates the examples a bit. One thing that I did do here after experimenting within the NextJS example was integrate the suspense boundary directly into
<Media>so that we don't have any forced breaking changes in the API and the library behaves as it did before.In my testing (and updated in this PR's examples),
<Suspense>still works as one would expect within the children of<Media>, even though<SuspenseBoundary>is contained higher-up. I know Dan mentioned that this is a UI thing and that<Suspense>should only ever be invoked by library consumers in userland, but in this case it makes sense to keep it in the lib and things (seem) to work just fine. We should track changes to React and update accordingly.@gurkerl83 - do you have any strong objections to this?
To test:
and then in another tab
cd examples/nextjs yarn link @artsy/fresnel yarn devWill update tests tomorrow and cut a new release.