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

feat(react-18): Add modern streaming implementation #14902

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

damassi
Copy link
Member

@damassi damassi commented Nov 23, 2024

The type of this PR is: Feat

Description

This one is quite exciting, because I wasn't sure if it was possible with styled-components, due to the core team waiting on the react team to release some new APIs for interleaving CSS within streams using the latest renderToPipeableStream. But to our luck, someone in the community just released an example app solving this problem last week and I tested it out here and things seem good ❇️

ENV Var

  • hokusai staging env set ENABLE_SSR_STREAMING=true
  • hokusai production env set ENABLE_SSR_STREAMING=true

To test in dev, set ^ in .env file.

For an overview on the powerful React 18 streaming features, see https://www.patterns.dev/react/react-selective-hydration/.

Log of data streaming in:

stream-data.mp4

There's still some more work to do to optimize for first render but with React 18 + renderToPipeableStream in place we can start implementing that. A couple of ideas:

  • Hoist the page shell up above our router so that renders immediately before any network requests get sent out. Right now we're still limited by our routing / app framework all being one thing, so the baseline min render time is still constrained by the overall time it takes to resolve data from MP, for whatever route we're currently landing on
  • Optimize all of our system query renderers to take advantage of Suspense. Right now we're only rendering on the client, but now that server-side suspense boundaries are supported we can
    • wrap SystemQueryRenderer in a proper suspense boundary
    • fire off the request on the server (async to page load). Component now suspends, via Relay
    • and then once the data is done fetching (in parallel to main page load), stream and rehydrate component on the client leading to much more instant page renders (vs current flow which is: SSR, render, mount, system query renderers render on client, fetch, show loading skeleton, resolve, then render correct UI)
    • ☝️ now works because of React 18's new architecture. We no longer have to wait for the whole page to become interactive; this is what selective hydration is all about.

In terms of our system query renderers, some work remains to implement that - ie, we'd need to write the SSR query renderer data to some kind of dom data-bridge and then read that from the client (rehydrate), so that double fetches don't occur once the client remounts. Very doable! React team's forward-thinking engineering wins again.

cc @artsy/diamond-devs

Copy link

relativeci bot commented Nov 23, 2024

#1042 Bundle Size — 9.56MiB (+0.13%).

112329a(current) vs ffe4f84 main#484(baseline)

Important

Bundle introduced 1 and removed 4 duplicate packages – View changed duplicate packages

Warning

Bundle introduced 3 new packages: web-vitals, @sentry-internal/browser-utils, stylis – View changed packages

Bundle metrics  Change 8 changes Regression 1 regression Improvement 3 improvements
                 Current
#1042
     Baseline
#484
Improvement  Initial JS 3.69MiB(-6.57%) 3.95MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 87.82% 2.04%
Change  Chunks 142(-0.7%) 143
No change  Assets 146 146
Change  Modules 5697(+1.06%) 5637
Regression  Duplicate Modules 501(+10.11%) 455
Change  Duplicate Code 6.12%(+4.08%) 5.88%
Improvement  Packages 282(-3.09%) 291
Improvement  Duplicate Packages 39(-7.14%) 42
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
#1042
     Baseline
#484
Regression  JS 9.35MiB (+0.39%) 9.31MiB
Improvement  Other 213.41KiB (-10.27%) 237.84KiB

Bundle analysis reportBranch streaming-3Project dashboard


Generated by RelativeCIDocumentationReport issue

@damassi damassi force-pushed the streaming-3 branch 2 times, most recently from e3011a4 to 966a012 Compare November 23, 2024 19:03

// Abandon and switch to client rendering if enough time passes.
streamTimeout = setTimeout(() => {
abort()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joeyAghion - Aside from better (general) error handling in React 18, hopefully this will solve any lingering issues we might encounter related to those datadog graphs with the React 17 implementation.

@damassi damassi force-pushed the streaming-3 branch 4 times, most recently from 908eb4a to c190a0d Compare November 24, 2024 04:44
@@ -10,7 +10,7 @@ import { useRouter } from "System/Hooks/useRouter"
import { FlashBanner_me$data } from "__generated__/FlashBanner_me.graphql"

interface FlashBannerProps {
me?: FlashBanner_me$data
me?: FlashBanner_me$data | null
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Just fixing this old annoying console warning)

@@ -159,6 +159,7 @@ export const NavBarLoggedInActionsQueryRenderer: React.FC<React.PropsWithChildre
<FallbackErrorBoundary FallbackComponent={Placeholder}>
<SystemQueryRenderer<NavBarLoggedInActionsQuery>
environment={relayEnvironment}
placeholder={user ? <Placeholder /> : undefined}
Copy link
Member Author

@damassi damassi Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed bug where nav flickers when checking whether logged in (after you've already logged in, on full page reload)

@damassi damassi force-pushed the streaming-3 branch 4 times, most recently from 56befce to 47ca53e Compare November 24, 2024 15:59
@damassi damassi force-pushed the streaming-3 branch 5 times, most recently from 8951a05 to 90e0acb Compare November 24, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant