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

Update React experimental version #758

Merged
merged 18 commits into from Mar 18, 2022
Merged

Conversation

frandiox
Copy link
Contributor

@frandiox frandiox commented Feb 25, 2022

Description

This PR upgrades to Hydrogen to the latest version of React experimental. This includes the latest changes to the Fizz streaming SSR API. It also includes server context, which has an impact on our pseudo server-only-context utilities.

We need to add these Suspense boundaries to prevent this hydration error:

Uncaught Error: This Suspense boundary received an update before it finished hydrating. This caused the boundary to switch to client rendering. The usual way to fix this is to wrap the original update in startTransition.

I'm tracking this issue in #920 so we can resolve it later.


Before submitting the PR, please make sure you do the following:

  • Run yarn changeset add to update the changelog, if needed
  • Read the Contributing Guidelines
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
  • Update docs in this repository for your change, if needed

@jplhomer
Copy link
Contributor

jplhomer commented Feb 25, 2022

I did a deep-dive on this and narrowed it down to an issue with React trying to hydrateRoot when:

  • streaming SSR contains Suspense fallbacks, resolved eventually with script tags
  • RSC response is streamed in and contains those same Suspense fallbacks

To isolate and reproduce, replace App.server.jsx with:

import renderHydrogen from '@shopify/hydrogen/entry-server';
import {ShopifyProvider, useShopQuery} from '@shopify/hydrogen';
import {Suspense} from 'react';
import shopifyConfig from '../shopify.config';

function App() {
  return (
    <ShopifyProvider shopifyConfig={shopifyConfig}>
      <p>Hello:</p>
      <Suspense fallback="Loading shop name...">
        <ShopName />
      </Suspense>
    </ShopifyProvider>
  );
}

function ShopName() {
  const {
    data: {
      shop: {name},
    },
  } = useShopQuery({query: `query ShopName { shop { name }}`});

  return <p>{name}</p>;
}

const pages = import.meta.globEager('./pages/**/*.server.[jt](s|sx)');

export default renderHydrogen(App, {pages});

And disable writing RSC script chunks in entry-server.tsx:

// bufferReadableStream(rscToScriptTagReadable.getReader(), (chunk) => {
//   log.trace('rsc chunk');
//   return response.write(chunk);
// });

And fix an issue in rsc.ts:

if (typeof __flight !== 'undefined' && __flight && __flight.length > 0) {

What happens is:

  1. Streaming SSR response contains Suspense fallback
  2. Streaming SSR replaces fallback with ShopName contents
  3. RSC endpoint returns the Suspense fallback. At this point, the Suspense fallback is re-rendered in the browser. It's also presumably when the Hydration fails, we get the error logged, and it defers to client rendering.
  4. RSC endpoint resolves the Suspense fallback.
Screen.Recording.2022-02-25.at.3.49.46.PM.mov

What I've tried

@frandiox
Copy link
Contributor Author

Thanks a lot for checking it, Josh! I couldn't find a workaround either. I guess we will need to report it in React repo and see if they have some pointers.

@salazarm
Copy link

If you add the onRecoverableError option does it tell you where the mismatch is occurring? Also if you go before this PR facebook/react#22629 the flickering would go away.

@jplhomer
Copy link
Contributor

@salazarm I don't think it gives us much information:

Error: This Suspense boundary received an update before it finished hydrating. This caused the boundary to switch to client rendering. The usual way to fix this is to wrap the original update in startTransition.
    at updateDehydratedSuspenseComponent (react-dom.development.js:22052:89)
    at updateSuspenseComponent (react-dom.development.js:21680:20)
    at beginWork (react-dom.development.js:22945:14)
    at beginWork$1 (react-dom.development.js:28020:14)
    at performUnitOfWork (react-dom.development.js:27168:12)
    at workLoopConcurrent (react-dom.development.js:27154:5)
    at renderRootConcurrent (react-dom.development.js:27116:7)
    at performConcurrentWorkOnRoot (react-dom.development.js:26347:38)
    at workLoop (scheduler.development.js:266:34)
    at flushWork (scheduler.development.js:239:14)

@salazarm
Copy link

In DEV, If you check the properties of the Error object can you see a componentStack ? cc @acdlite

@jplhomer
Copy link
Contributor

jplhomer commented Mar 1, 2022

I don't see componentStack on that Error, just stack.

@acdlite
Copy link

acdlite commented Mar 2, 2022

This error happens when there's a synchronous update during hydration. This can happen in a few different ways, but the most common one is if when something calls setState inside of useLayoutEffect. That will cause everything below that component to synchronously finish. Not sure if that's what's happening in your case, but having seen this many times before I'd say it's likely.

We don't currently track expose information about which update caused the synchronous re-render. DevTools is working on a feature that does this, though. @bvaughn Did the "update tracking" feature ship yet? Is there some way we could try it out to debug this issue?

In the meantime, I would check for updates inside useLayoutEffect because that's often what this ends up being.

@frandiox
Copy link
Contributor Author

frandiox commented Mar 3, 2022

Thanks for helping here!

This error happens when there's a synchronous update during hydration. This can happen in a few different ways, but the most common one is if when something calls setState inside of useLayoutEffect

Afaik, we don't use useLayoutEffect anywhere in our code.

However, I found something similar to what you described: Shopify/hydrogen@453e2ef (I guess we could use startTransition here but this fix looks simpler in this case).

This change fixed the This Suspense boundary received an update before it finished hydrating issue, but then many mismatch errors started to appear. I've added extra Suspense boundaries in bcce76a which seem to fix it... but I've no idea why some of them are required 🤔

@bvaughn
Copy link

bvaughn commented Mar 3, 2022

We don't currently track expose information about which update caused the synchronous re-render. DevTools is working on a feature that does this, though. @bvaughn Did the "update tracking" feature ship yet? Is there some way we could try it out to debug this issue?

The "legacy" Profiler shows which Fiber(s) scheduled a commit using the memoizedUpdaters field, which was added in facebook/react#15658. This didn't make it into v17 but it is in the 18 RC.

The new Timeline profiler could also be helpful here as it shows state updates and the name of the component scheduling the update (unfortunately not the component stack, yet) in context with other info (like JS call stack). That's also available if you're using the latest DevTools and the 18 RC.

@frandiox
Copy link
Contributor Author

frandiox commented Mar 3, 2022

@salazarm @acdlite It looks like renderToReadableStream (fizz, consuming flight response) is bugged in 0.0.0-experimental-17806594c-20220301 and newer versions (in Cloudflare Workers). It passes Cannot enqueue a zero-length ArrayBuffer. error to onError and onCompleteAll is never called (the ReadableStream is never closed either).

I was under the impression that onError hook was called with recoverable errors so we were not terminating the response there. However, the previous Cannot enqueue a zero-length ArrayBuffer. made it hang because the RS is never closed. Should we generally terminate the response whenever onError is called even if it's in the middle of a streaming?

I've downgraded to 0.0.0-experimental-f468816ef-20220225 and it seems everything works again.

@gurkerl83
Copy link

Hi all, hydration changed in experimental React 18 versions about a month ago completely. What you describe here looks very similar to an issue we recently opened here. facebook/react#23381

@frandiox frandiox marked this pull request as ready for review March 7, 2022 13:22
@frandiox frandiox requested a review from jplhomer March 7, 2022 13:23
@github-actions

This comment has been minimized.

@frandiox frandiox changed the title Update to latest React experimental Update React experimental version Mar 7, 2022
Copy link
Contributor

@jplhomer jplhomer left a comment

Choose a reason for hiding this comment

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

A couple questions. Could you also add a changeset with instructions for upgrading to the correct new React experimental version?

packages/hydrogen/src/foundation/Router/Router.client.tsx Outdated Show resolved Hide resolved
jest-setup.ts Outdated Show resolved Hide resolved
@jplhomer jplhomer added the blocked Can't continue until something else is fixed label Mar 8, 2022
@jplhomer jplhomer changed the base branch from main to v1.x-2022-07 March 8, 2022 21:37
@frandiox frandiox removed the blocked Can't continue until something else is fixed label Mar 16, 2022
@frandiox frandiox requested a review from jplhomer March 16, 2022 10:29
@frandiox
Copy link
Contributor Author

frandiox commented Mar 16, 2022

@jplhomer Can we revisit this? There are still a few extra Suspense but in general I don't see that many mismatches or flashes anymore: https://hydrogen-rc.frandiox.workers.dev/

It's updated to the latest version released 2 days ago. I've also updated the Vite RSC plugin and everything seems to work. Can you please check?

If we merge this we can start using server context 🎉

Copy link
Contributor

@jplhomer jplhomer left a comment

Choose a reason for hiding this comment

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

Going to merge this since it resolves the hydration errors we're seeing in the main branch, and it gets us on the most recent experimental release.

We are still seeing Suspense errors in some places, and the temporary fix (which is in this PR) is to wrap client components with Suspense boundaries. We haven't nailed down exactly why this is happening yet. I can generally reproduce it during SSR when a server component suspends, we stream the fallback to the client, and then resolves containing other client components. If the client module references have not yet been flushed to the RSC stream (which is a bunch of script tags being interleaved in the SSR stream in our implementation), this causes a hydration error. This seems to align with our fix of wrapping those client components with suspense boundaries.

It's difficult to get this into a standalone repro, but I'll try to get something to you soon to look at @acdlite and @salazarm! Appreciate your help in this PR ❤️

@jplhomer jplhomer merged commit 0bee3af into v1.x-2022-07 Mar 18, 2022
@jplhomer jplhomer deleted the fd-update-react-experimental branch March 18, 2022 13:39
@jplhomer jplhomer mentioned this pull request Mar 18, 2022
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

Successfully merging this pull request may close these issues.

None yet

6 participants