fresnel icon indicating copy to clipboard operation
fresnel copied to clipboard

feat(react-18): Add official React 18 SSR support

Open damassi opened this issue 1 year ago • 5 comments

Closes https://github.com/artsy/fresnel/pull/289 Closes https://github.com/artsy/fresnel/issues/260

All credit goes to @gurkerl83 and the work he spearheaded in #289 and the investigations in https://github.com/artsy/fresnel/issues/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:

yarn link
yarn watch 

and then in another tab

cd examples/nextjs
yarn link @artsy/fresnel 
yarn dev

Will update tests tomorrow and cut a new release.

damassi avatar Aug 12 '22 00:08 damassi

@damassi if you have time, could you please adjust the pr with the comments I made and provide a new release. Thx!

gurkerl83 avatar Aug 15 '22 19:08 gurkerl83

@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 avatar Aug 15 '22 19:08 damassi

@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…

gurkerl83 avatar Aug 15 '22 19:08 gurkerl83

If you can hit the 'finish review' button then i'll see your comments; i can't see them until they've been posted.

damassi avatar Aug 15 '22 19:08 damassi

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.

gurkerl83 avatar Aug 15 '22 19:08 gurkerl83

but I see a few of mine in pending state

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:

Screen Shot 2022-08-15 at 1 34 21 PM

Other than that, not too sure. Maybe try re-adding your comments and posting them as single comments, per this button?

Screen Shot 2022-08-15 at 1 35 56 PM

Forgive me if i'm misunderstanding! But i can't see your PR comments as it is, and thus can't address them.

damassi avatar Aug 15 '22 20:08 damassi

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 🙏 💯

damassi avatar Aug 15 '22 23:08 damassi

:rocket: PR was released in v5.0.0 :rocket:

artsyit avatar Aug 15 '22 23:08 artsyit