vercel / next-react-server-components

Demo repository for Next.js + React Server Components
https://next-rsc-hn.vercel.app
MIT License
978 stars 156 forks source link

Document or fix known issues with this demo #13

Open gaearon opened 2 years ago

gaearon commented 2 years ago

I'm starting to see this demo being used in performance comparisons in a way that implies this is the canonical RSC experience. It would be good to either solve or document (in README) concrete issues with it so that a future reader has an idea of what's correct and what's a known issue. (I've added https://github.com/reactjs/server-components-demo/pull/57 to the original demo in the same spirit.)

Update

The up-to-date list of problems (likely non-exhaustive) is here: https://github.com/vercel/next-rsc-demo/issues/13#issuecomment-988438962


Streaming "Jank"

This demo shows each story "streaming in" individually:

https://user-images.githubusercontent.com/810438/145104379-12b00f2d-9eb9-4434-b98b-b48036aa43ad.mov

This is a rather janky user experience.

I am guessing that this demo does it to make the "streaming" aspect obvious. However, I am observing that people take away that this jankiness is what we mean by "streaming". It seems like a rather unfortunate consequence if people's takeaway becomes that apps built with Server Components have pieces randomly "popping in" — especially because the whole point of the <Suspense> model is to give you precise control over what's allowed to "pop in" individually vs what pieces must "pop in" together.

How to fix it?

If I understand correctly, there are multiple ways to fix this:

Additional server request

I am seeing an extra request to the RSC entry point from the client after the page loads:

Screenshot 2021-12-07 at 21 38 56

I wouldn't expect the initial page load to need to make any additional requests. Do we understand why it happens? Is this a bug or a known limitation of the current demo?

Thanks

I very much appreciate the work on this demo. I'm hoping we can fix and/or document the issues in it so that people are aware what's missing and don't make an impression of the overall RSC architecture or user experience based on an early demo.

Thanks!

ryansolid commented 2 years ago

Also I noticed a lot of <script> tags with defer and none with async. That would push off hydration to right before the document load event ie.. everything is done streaming in. Not sure if there is a technical reason but it seems maybe not right. Ignore me if I'm off base.

shuding commented 2 years ago

Thank you for opening this issue Dan!

Same as what you mentioned in reactjs/server-components-demo#57, this demo isn’t intended to reflect the performance or behavior of a real app too. We will add a note in the README soon.

The main goal of this demo was to demonstrate the “streaming” feature, so there are noticeable spinners and even some manual slowdowns. We’ve considered <SuspenseList> but it isn’t ready yet so we went with the most basic way.

For the additional server request and the defer attribute of <script> (thanks @ryansolid!), we are already tracking them in https://github.com/vercel/next.js/issues/30994 and https://github.com/vercel/next.js/issues/31338. They’re also included in our roadmap.

Thanks again!

gaearon commented 2 years ago

The main goal of this demo was to demonstrate the “streaming” feature, so there are noticeable spinners and even some manual slowdowns.

This seems unfortunate because it conflates streaming of data (good) with unpredictable streaming of UI (bad). Perhaps HN is not the right app to demonstrate this since no reasonable designer would place a spinner around every link. My worry is that this demo creates a perception that streaming of UI is essentially uncontrollable.

Do we have any ideas on how to resolve this? Is streaming the whole “page content” at once (but separate from the page shell) not sufficient to demonstrate streaming? Is this example fundamentally flawed as a streaming demo? Come to think of it, it seems rather inefficient that we assemble the front page from 20 requests anyway. I don’t believe a real HN implementation would work this way. Was the data fetching split into multiple requests for the purpose of this demo, or is requesting individual items the only API exposed by HN?

gaearon commented 2 years ago

Let me try to rephrase my concern from the other end. I understand the intention is to demonstrate streaming. However, from the user’s perspective, the purpose reads as “build the best possible HN client with RSC”. Similar to how a framework’s TodoMVC reads as a canonical way to build TodoMVC in it, regardless of the author’s intent.

So any flaws in this example (especially significant flaws in the UX) will be taken as flaws in the paradigm itself — especially now that the demo exists “as is” without the surrounding keynote. I’ve spoken to a few people and they were all surprised that this demo isn’t meant to show “best practice”.

How can we address this?

shuding commented 2 years ago

Come to think of it, it seems rather inefficient that we assemble the front page from 20 requests anyway. I don’t believe a real HN implementation would work this way. Was the data fetching split into multiple requests for the purpose of this demo, or is requesting individual items the only API exposed by HN?

Unfortunately it is the only way with the public API of HN I can find. One good way to improve the demo is to only show the spinner for the list container but not the individual items, as you proposed. And when there is <SuspenseList> we can then switch to use it for a even better experience.

gaearon commented 2 years ago

One good way to improve the demo is to only show the spinner for the list container but not the individual items, as you proposed.

Can I ask you to look into this? I imagine y’all are pretty busy but this seems important, and the issues I noted while trying to do it myself also seem a bit worrying. So it seems like maybe this could also help uncover some bugs in the process. Thanks for considering — and really appreciate such a snappy response.

shuding commented 2 years ago

Fixed in https://github.com/vercel/next-rsc-demo/pull/15 and you saw it already :) Thanks for the help BTW!

carlosagsmendes commented 2 years ago

I'm sorry to chime in, but I couldn't help to imagine Andrew Clark's reaction to so many spinners when I saw this demo for the first time (and I don't know him, I just thought of his Suspense presentation about the "spinner hell"). After seeing the code, I think I got the idea from a technical point. But from an end user's experience, this is not great to impress any "boss" to adopt RSC. Something like the "canonical" Notes App running in NextJS would be more helpful and educational.

shuding commented 2 years ago

Something like the "canonical" Notes App running in NextJS would be more helpful and educational.

Yes we are working on that one as well. Thanks for the great feedback here :)

gaearon commented 2 years ago

As a follow-up, here's a list of the issues I'm currently aware of in this demo:

shuding commented 2 years ago

Thanks for putting those together @gaearon!

For those who want to track updates of these items, or jump into more detailed technical discussions, here's a list of these issues (and how are we gonna resolve them) in the Next.js repo. So feel free to subscribe or share feedback there:

devknoll commented 2 years ago

It would be great if there was some way to try a "fully buffered" version of RSC in the meantime. So that this demo can be compared apples-to-apples to other non-streaming solutions and we can see if there are other inefficiencies in React itself that we missed because of distractions like missing compression.

@gaearon I think it would be preferable if React were (even more) adaptive to backpressure (cc @sebmarkbage). Then, this sort of this could be achieved by just supplying constant backpressure in a server that doesn't support streaming.

We previously opened https://github.com/facebook/react/pull/22726 to address one part of this, to prevent React from starting to write to a stream if it's already full. Another PR that would be needed is for React to write to the buffer and close the buffer regardless if it's full, if the render is completely finished.

(We also opened https://github.com/facebook/react/pull/22350 to add our own stream abstraction to work around some of these things ourselves, but it has gone stale)

shuding commented 2 years ago

Hey everyone! To give an update here, most of the critical issues mentioned above by @gaearon have been fixed in the latest Next.js, and the demo is also upgraded to use react@rc:

Let me know if you have any questions!

shuding commented 2 years ago

Another update: compression is now enabled automatically, the demo is also upgraded to React RC.1.

gaearon commented 1 year ago

More issues with this demo.

Solved ## Hydration errors There are intermittent hydration errors: Screenshot 2023-04-28 at 16 28 08 It seems like they're due to the timestamps: Screenshot 2023-04-28 at 16 28 54 I think you want to add `suppressHydrationWarning` to them if these are Client components. ## Disorienting scroll behavior This doesn't reproduce every time, but the scroll behavior is bizarre. When I click "next page", I sometimes see the "middle" of the Suspense fallback: https://user-images.githubusercontent.com/810438/235190972-ecf7a215-68bc-4809-8ba8-8993d21e44d5.mov But sometimes it jumps right to the top immediately. I can't quite catch the pattern. ## Extra RSC request for first page load Why does this happen? The initial data is already inlining. Why is it refetching it again? Screenshot 2023-04-28 at 16 28 02 ## Refetch on Back Button Back button isn't supposed to refetch. But it does. Why? https://user-images.githubusercontent.com/810438/235193332-54777b16-48ae-4e51-9740-6fc27357bd7f.mov

Poor Suspense fallback (?)

In general, I don't think this Suspense fallback makes sense for individual pages.

https://user-images.githubusercontent.com/810438/235192289-77676988-fb5f-485a-804f-19b3e88e6274.mov

It feels pretty disruptive and not very well thought-out. (It doesn't even match the shape of the content about to appear.)

Can we remove it altogether and just have the transitions "wait" as the first step? And then maybe re-add Suspense fallback only around comments? And make it intentional.

Inconsistent Triggering of Suspense

Notice that the first time I click "More", I see a fallback. But next times I don't. What's up with that?

https://user-images.githubusercontent.com/810438/235192798-38f723b9-0801-42f7-a1f7-bbf6319fa555.mov

gaearon commented 1 year ago

@huozhi Do you plan to work on other items or do you need some help there? Would be nice to fix before there's renewed interest.

gaearon commented 1 year ago

I sent a PR for the loading stuff. https://github.com/vercel/next-react-server-components/pull/67

huozhi commented 1 year ago

@gaearon Thanks for bringing up these issues, the hydration errors is already patched. Will take a look at the rest ones with team 🙏

timneutkens commented 1 year ago

Disorienting scroll behavior This doesn't reproduce every time, but the scroll behavior is bizarre. When I click "next page", I sometimes see the "middle" of the Suspense fallback:

I've fixed this in https://github.com/vercel/next.js/pull/48986, details on why it happened are on the PR. Upgraded the demo to the latest version.

gaearon commented 1 year ago

I've merged #67 which updates to latest canary.

Let's see what other issues are left.

Weird scroll on small screen heights

To repro, use small screen height.

https://user-images.githubusercontent.com/810438/236010333-f1f9acca-0f65-4586-9939-21b12678e7e3.mov

Disappearing content

  1. Go to https://next-rsc-hn.vercel.app/item/35803435
  2. Press "Hacker Next" in header

Expected: it waits for content, then navigates to the first page. Pressing back navigates back to the story. Actual: it renders first page with a "hole" for stories. But there was no Suspense boundary. Pressing back navigates to... the front page again. It's like there's an extra navigation in the history.

This is probably related:

Screenshot 2023-05-03 at 20 29 54

Can't fetch a large page

Note: this doesn't repro after https://github.com/vercel/next-react-server-components/commit/c7c6818224d2b718a314b8b9a6a2b09b6310a2b8, but presumably it's still a bug since you might want prefetch={true}. Assuming c7c6818224d2b718a314b8b9a6a2b09b6310a2b8 is temporarily reverted, open https://next-rsc-hn.vercel.app/item/35771104. It errors ("Application error: a server-side exception has occurred"). Not sure if this is just way too many comments and we're hitting some kind of rate limit.

Too eager prefetching

If you look at https://next-rsc-hn.vercel.app/ now, it seems clear it actually prefetches comments now. This seems to only happen for prod builds. But it's not supposed to because comments have a Loading boundary. That seems like a bug.

Fixed by https://github.com/vercel/next-react-server-components/commit/c7c6818224d2b718a314b8b9a6a2b09b6310a2b8.

omerman commented 4 months ago

As an additional optimization on top of this inlining, we'll want to deduplicate text content between them. E.g. we shouldn't need to repeat all the story titles in the Flight data structure even though they have just appeared in the HTML itself. We need to add a capability in React to "skip" specially marked Flight text and keep using the one already in the DOM, thereby letting us deduplicate it.

Is this part being considered? Or are the remaining tasks abandoned? 😢

leerob commented 2 months ago

I believe everything in this thread is now addressed (repo is on the latest Next.js version now as well). Anything I'm missing?