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

Bug: startTransition not working with useSyncExternalStore #24810

Closed
stanislav-halyn opened this issue Jun 29, 2022 · 12 comments
Closed

Bug: startTransition not working with useSyncExternalStore #24810

stanislav-halyn opened this issue Jun 29, 2022 · 12 comments
Labels
Component: Concurrent Features Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@stanislav-halyn
Copy link

stanislav-halyn commented Jun 29, 2022

SSR content shows a fallback during hydration if there has been an update to an external state, even if wrapped with startTransition.

React version: 18.2.0

Steps To Reproduce

  1. Create a basic SSRed React application that would have suspensible content.
  2. Create an external store and read its values using useSyncExternalStore in the same component which has suspensible content.
  3. Update this external store on the application mount(while hydrating), and wrap this update with startTransition;

Link to code example: https://codesandbox.io/s/react-18-starttransition-not-working-with-usesyncexternalstore-4pxygp

The current behavior

The content shows a fallback while the update is done.

The expected behavior

The content should not show a fallback, but rather update without showing it.

@stanislav-halyn stanislav-halyn added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Jun 29, 2022
@stanislav-halyn
Copy link
Author

As noted in useSyncExternalStore's discussion, it seems that startTransition was never intended to work with external store updates.

But this still leaves a question, how do I avoid showing a fallback during hydration if I need to update the store on the component's mount?

@SuperOleg39
Copy link

@stanislav-halyn hi!

I recently met a similar problem, and found answer here - #24476 (comment)

In this example, you don't pass props from user store to suspended component directly.
So, just memoize this suspended block:

export default function Layout() {
  const user = useUser();

  useEffect(() => {
    startTransition(() => {
      User.updateUser({
        data: {
          firstName: "Steve"
        }
      });
    });
  }, []);

  const memoizedContent = useMemo(
    () => (
      <Suspense fallback={<Loader />}>
        <LazyContent />
      </Suspense>
    ),
    []
  );

  return (
    <div>
      <header>this is header</header>
      <br />
      <p>user data: {JSON.stringify(user.data)}</p>.
      <br />
      <main>{memoizedContent}</main>
    </div>
  );
}

@gaearon
Copy link
Collaborator

gaearon commented Sep 20, 2022

In general, updating store “on mount” isn’t a great pattern. Think about it: from the user’s perspective, there was no interaction. Nothing really “changed” in the app. The user did not provide any new information. The app didn’t download any new information. So why does it need to update? What new information is there that hasn’t been there since the beginning?

I think you’ll find that with some restructuring, you should be able to get rid of store updates “on mount”. That will both solve this problem and improve performance in general.

@gaearon gaearon closed this as completed Sep 20, 2022
@stanislav-halyn
Copy link
Author

@gaearon Thanks for your reply!
The example I gave is simplified, because in our application we fetch user's data on the component's mount and update user's store with the fetched data. Nevertheless this still produces react's suspense error.

I don't know any other way of fetching user's data, but on the component's mount.

In the end, the recommended way of using startTransition in such cases didn't work, so we ended up fetching user's data on the component's second render which works, but I don't find it a good solution.

@gaearon
Copy link
Collaborator

gaearon commented Sep 22, 2022

The recommended solution with SSR is to fetch data during SSR rather than on mount. Otherwise, SSR somewhat loses its purpose — the app's HTML is a shell that has no useful content. I wonder if you've considered this option? This is what frameworks like Next.js usually do.

@stanislav-halyn
Copy link
Author

stanislav-halyn commented Sep 22, 2022

@gaearon in our case we don't render on SSR the whole page content, but only a part of this, which is shared between users.

This enables us to use shared cache between users, but disables us from fetching user's data on SSR.

So basically in our situation we have to fetch some data on components mount rather than during SSR, because we don't need to render everything during SSR and want to use shared cache between users

@gaearon
Copy link
Collaborator

gaearon commented Sep 22, 2022

OK, got it. It's a little unfortunate situation but I guess the fix would be to memoize the content to "prove" that it doesn't depend on the data you just fetched, and so it is safe for React to preserve the server HTML.

https://codesandbox.io/s/react-18-starttransition-not-working-with-usesyncexternalstore-forked-mxm3u0?file=/src/Layout.js

const MainContent = memo(function MainContent() {
  return (
    <main>
      <Suspense fallback={<Loader />}>
        <LazyContent />
      </Suspense>
    </main>
  );
});

A more canonical fix would be to SSR the entire page for every user, and then do data fetching on the server. I see your point re: caching, but I'm curious if you need to cache render output or shared data for users. Because if it's enough to cache some shared data then you can still have a shared cache between separate SSR requests. However, if it's important to cache render output, you'd have to wait for React to add special support for caching between SSR. It's something we've been exploring but it's not likely to come very soon.

@stanislav-halyn
Copy link
Author

@gaearon thanks for the suggestion :)
I wonder if LazyContent internally uses the user data from the store, will it fail hydration if we update the store on mount somewhere up the tree? Or will it fail only if we use the user's data directly in the MainContent component?

Unfortunately, we cache render output and we can't change that.

@gaearon
Copy link
Collaborator

gaearon commented Sep 22, 2022

I wonder if LazyContent internally uses the user data from the store, will it fail hydration if we update the store on mount somewhere up the tree?

We can preserve server HTML when we have a guarantee that an update at the top does not flow through the dehydrated component. So, there are two ways to break this guarantee:

  • Re-render it with new props (so that memo can't bail out)
  • Update context above it

If either of those things happen, we'll have to replace HTML with the fallback.

So, to answer your question, updating the store above by itself is not a huge problem as long as you memoize the lazy content. However, if you pass the store content as a prop to the lazy content, it would have to be replaced (because preserving the server HTML is not safe anymore — e.g. imagine a Theme toggle: you don’t want to see half of the app use a Dark theme and another half use the Light theme). Similarly, if you put the store state into a Context Provider above the lazy content, this would cause the lazy content HTML to be replaced. This is because until we load the code, we can't know whether the lazy content reads that context. Therefore, we have to replace it if any context changes above it.

@gaearon
Copy link
Collaborator

gaearon commented Sep 22, 2022

Note there's also another option, which is to not SSR LazyContent either. I.e. you can always emit fallback on the server. Then it won't be in HTML (which is unfortunate), but you'll also avoid this problem. If you decide to go that route, you need to throw an Error on the server only from inside the part you want to make client-only (like this lazy content). When you throw an Error on the server, with streaming SSR, the error won't fail the render. Instead, the server will emit the Suspense fallback. The client will retry rendering when the code loads. During retry, you won't throw, so lazy content will be able to show up. The error will be reported in the console, but you can pass onRecoverableError with hydrateRoot options, detect your custom Error (e.g. by its message) and silence it because it's expected. This is a supported way to render client-only content.

@OliverJAsh
Copy link

OliverJAsh commented Oct 12, 2022

updating store “on mount” isn’t a great pattern

@gaearon At Unsplash we are using this pattern quite a bit, specifically where we need to store a value in Redux that can only be known on the client-side. For example, the window width or a value that is derived from local storage. These value can not be determined during SSR, and we can't update the store before the first client render because this would result in a "hydration mismatch" whereby the first client render doesn't match the server render. For this reason we dispatch an action on mount to update the store, then React can do an additional render. I don't see any other way of doing this but maybe I'm missing something.

For context: I'm coming here from reduxjs/react-redux#1962.

We can workaround this issue by using React.memo but this seems like a hacky solution.

@max-ch9i
Copy link

So, there are two ways to break this guarantee:

Re-render it with new props (so that memo can't bail out)
Update context above it

Another case is when useSyncExternalStore is used. In this example https://stackblitz.com/edit/nextjs-yuqb6g?file=app%2FPageComponents.js , the store is updated before Suspense renders a component that is subscribed to the store with useSyncExternalStore.

How can the hydration error be avoided in this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Concurrent Features Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

6 participants