Skip to content
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

Document or fix known issues with this demo #13

Open
2 tasks
gaearon opened this issue Dec 7, 2021 · 22 comments
Open
2 tasks

Document or fix known issues with this demo #13

gaearon opened this issue Dec 7, 2021 · 22 comments

Comments

@gaearon
Copy link
Collaborator

gaearon commented Dec 7, 2021

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 reactjs/server-components-demo#57 to the original demo in the same spirit.)

Update

The up-to-date list of problems (likely non-exhaustive) is here: #13 (comment)


Streaming "Jank"

This demo shows each story "streaming in" individually:

bad_ux.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:

  • Remove this <Suspense> boundary so that all stories "pop in" at once. Despite delaying the display, this is a much better user experience because there's no content layout shift, and individual jumping rows aren't useful anyway.

    • I think you'll also want to replace the "top-level" spinner with null and make the body full-height so that the loading sequence before we get the stories isn't jumpy either. Ideally we'd either hold the entire page or show the shell that will be "filled in" — both of these could feel natural. A small body with a spinner in the top left corner looks weird.
    • I've tried doing that, but ran into a hydration error I couldn't fix. Is that a bug in React? For some reason, the exact place where the error happens wasn't reported in the console so I've had a hard time trying to fix this. It seems like there needs to be an investigation for:
      • Why removing this <Suspense> boundary breaks hydration
      • Why errors are not being fully reported to the console
  • Alternatively, use <SuspenseList>, replace a spinner fallback with null, and hide the tail, so that you only see stories appearing "in order". This is currently out of question because <SuspenseList> won't be in 18.0 and will come later in 18.x. It's not currently implemented on the server.

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
Copy link

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
Copy link
Member

shuding commented Dec 7, 2021

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 vercel/next.js#30994 and vercel/next.js#31338. They’re also included in our roadmap.

Thanks again!

@gaearon
Copy link
Collaborator Author

gaearon commented Dec 7, 2021

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
Copy link
Collaborator Author

gaearon commented Dec 7, 2021

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
Copy link
Member

shuding commented Dec 7, 2021

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
Copy link
Collaborator Author

gaearon commented Dec 7, 2021

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
Copy link
Member

shuding commented Dec 7, 2021

Fixed in #15 and you saw it already :) Thanks for the help BTW!

@carlosagsmendes
Copy link

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
Copy link
Member

shuding commented Dec 7, 2021

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
Copy link
Collaborator Author

gaearon commented Dec 8, 2021

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

  • (Fixed) The spinners are creating a bad UX. Remove the Suspense boundary for individual items #15
  • I get an occasional page load on https://next-news-rsc.vercel.sh/rsc with no script tags, causing a crash. I couldn't repro it locally. Note that the stream completes with </body> but all script tags are missing. Crash details.
  • The compression is off for the main HTML response. This might be one of the reasons the initial paint appears so much slower. From what I gathered talking to @shuding, this is a limitation of the Edge Runtime that's currently turned on when you enable streaming in Next.js. It would be great to resolve this.
    • It would be great to have a way to run (at least locally!) with compression on so that we can test realistic perf.
  • There's an extra client request to the Flight endpoint, seemingly doing another round of data fetching before hydration. (If I understand correctly!) We should never need a client request after an SSR'd page loads. What I'd expect to happen instead is for the Flight response to be embedded into the HTML at the end (at a low priority?). Not sure if this requires some wiring in React itself but maybe. This is important for correctness too since you don't want to hydrate based on a different version of the response than the one that produced the initial HTML. Server Component response must be flushed inline with SSR next.js#30994
    • 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.
  • Fix the defer / async thing. Library Upgrade Guide: <script> (e.g. SSR frameworks) reactwg/react-18#114
  • 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.

@shuding
Copy link
Member

shuding commented Dec 8, 2021

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
Copy link

devknoll commented Dec 8, 2021

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 facebook/react#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 facebook/react#22350 to add our own stream abstraction to work around some of these things ourselves, but it has gone stale)

@shuding
Copy link
Member

shuding commented Dec 21, 2021

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:

  • /rsc with no script tags, causing a crash
  • There's an extra client request to the Flight endpoint
  • Fix the defer / async thing

Let me know if you have any questions!

@shuding
Copy link
Member

shuding commented Mar 9, 2022

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

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 28, 2023

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:

scrooll.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?

refe.mov

Poor Suspense fallback (?)

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

yo.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?

yooo.mov

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 28, 2023

@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
Copy link
Collaborator Author

gaearon commented Apr 28, 2023

I sent a PR for the loading stuff. #67

@huozhi
Copy link
Member

huozhi commented Apr 28, 2023

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

@timneutkens
Copy link
Member

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 vercel/next.js#48986, details on why it happened are on the PR. Upgraded the demo to the latest version.

@gaearon
Copy link
Collaborator Author

gaearon commented May 3, 2023

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.

wat.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 c7c6818, but presumably it's still a bug since you might want prefetch={true}. Assuming c7c6818 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 c7c6818.

@omerman
Copy link

omerman commented Feb 16, 2024

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
Copy link
Member

leerob commented May 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants