Add initial SSR support for React 18 and React-Redux v8

[markerikson]

This PR:

• Updates the context types to include a getServerState field
• Updates Provider to accept a serverState prop, and if included, pass down a getServerState callback that returns that value
• Updates useSelector to pass getServerState to uSES
• Identifies areas where connect is almost definitely abusing uSES :)

I'd like to get some eyes on what I've got so far and find out if I'm headed in the right direction overall. The changes here are based on comments made by @acdlite in various React 18 Working Group discussions.

I'm also interested in example tests we can use to verify that any of these changes are working as intended - please offer suggestions or PRs!

connect's logic is screwy because we need to know if the child props are being recalculated first, so that we either trigger a re-render or go ahead and cascade the subscription notifications down the tree. As a result, my current code still has connect doing all the actual meaningful comparison work, and then abusing uSES by passing down the "child props selector" function as the getSnapshot argument to uSES. The actual call to getState still happens within connect itself.

This has at least passed all our tests so far, but now that I'm trying to figure out how to potentially take the getServerState callback and pass it to uSES, I'm pretty sure all of this is basically incorrect usage :)

This suggests there are three possibilities:

• Some hypothetical fix that involves just shuffling pieces around and somehow it'll end up being sufficiently okay to work
• Completely rewriting the guts of connect + the mapState/mapDispatch logic (which has been basically as-is and untouched since it was written in late 2016) in order to better accomodate the revised requirements
• Leave connect as-is and say "look, it'll run okay, but if SSR hydration is your concern we just can't fully support that - it's too difficult. So, if you're still using connect you'll have to accept whatever fallback behavior React does here".

Paging interested parties:

@acdlite @timdorr @eps1lon @phryneas @dai-shi @Andarist

[netlify]

✔️ Deploy Preview for react-redux-docs ready!

🔨 Explore the source changes: ed8fdc8

🔍 Inspect the deploy log: https://app.netlify.com/sites/react-redux-docs/deploys/61c39aa66c5415000a62e758

😎 Browse the preview: https://deploy-preview-1835--react-redux-docs.netlify.app

[github-actions]

Size Change: +341 B (1%)

Total Size: 21.7 kB

Filename	Size	Change
dist/react-redux.js	16.6 kB	+286 B (1%)
dist/react-redux.min.js	5.12 kB	+55 B (1%)

_{compressed-size-action}

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ed8fdc8:

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration

[Andarist]

Do you plan to support server snapshots for a !didStoreComeFromContext case? I'm not interested in it myself but it would be a surprise to me that, as a user, I couldn't use this functionality when using a store prop

[markerikson]

Any changes in the connect file are entirely speculative at this point, because I don't know what I'm supposed to be doing here :)

[Ephem]

To support server snapshots here, connect would probably need to take a new serverState-option just like the Provider does. I think this is a niche use case, not sure if it warrants the complexity?

If this is not supported, passing store to connect could warn/error if serverState was passed to a Provider further up in the tree.

[Andarist]

q: when this can throw with useSyncExternalStore? shouldn't useSyncExternalStore be resilient to tearing etc that could make this sort of stuff throw pre-uSES?

[Andarist]

I wonder - do you maybe have a good reference for the getServerState snapshot API~? I've tried searching through WG discussions but I came empty-handed. I recall reading about this but would love to refresh myself on why this is needed with uSES and how React avoid the need for it with its primitives

[eps1lon]

I've tried searching through WG discussions but I came empty-handed.

facebook/react#22361 might help. Note that my initial post made some false assumptions that were corrected.

I recall reading about this but would love to refresh myself on why this is needed with uSES

By the time a particular component is hydrated, the store might've already been mutated. Simplest example I encountered was something like

<MutatesStoreInAnEffect />
<ReadsStore />
<React.Suspense>
  <ReadsStore />
</React.Suspense>

Assuming no component suspends, the server would render the same value for the store in every ReadsStore. But if you hydrate this tree then the second ReadsStore might be hydrated after the effects of MutatesStoreInAnEffect are run (due to selective hydration). The first ReadsStore would hydrate fine because we still have the initial state.

So we need an API to tell React with what state the markup was rendered on the server.

how React avoid the need for it with its primitives

Not sure I understand this statement correctly but React does need getServerSnapshot in every version. It needs it even more in React 18. In React 17 you'd be safe against hydration mismatch if your store is never mutated during render. In React 18 you can even get hydration mismatches if your store is mutated in effects.

[Andarist]

Thank you for the link and the explanation. This is what I was roughly expecting. I didn't clearly understand how the client value is handled though, and according to Andrew, the "mismatched" component is just re-rendered on top of the hydrated component that was hydrated based on the server snapshot.

Not sure I understand this statement correctly but React does need getServerSnapshot in every version. It needs it even more in React 18. In React 17 you'd be safe against hydration mismatch if your store is never mutated during render. In React 18 you can even get hydration mismatches if your store is mutated in effects.

I'm wondering about the situation when no external store is involved. Just replace MutatesStoreInAnEffect with UpdatesStateInAnEffect and ReadsStore with ReadsPropOrContextValueThatComesFromReactState in your example. This is basically the very same situation as the described by your, but useState doesn't support any getServerSnapshot method:
https://github.com/facebook/react/blob/5cccacd131242bdea2c2fe4b33fac50d2e3132b4/packages/react-reconciler/src/ReactFiberHooks.new.js#L2592-L2594

So I wonder, how the implementation is different for both and how React can handle this better for a plain state value?

[eps1lon]

So I wonder, how the implementation is different for both and how React can handle this better for a plain state value?

The issue includes this exact scenario (the codesandbox includes an implementation that uses context+state). How this is implemented I don't know and I'm pretty sure it's better I don't know. Though I suspect it has to do with individual fibers having an alternate or work-in-progress version (terminology might be off). But you can probably find out with the debugger and https://codesandbox.io/s/react-18-updating-store-in-an-effect-during-mount-causes-hydration-mismatch-uses-m6lwm?file=/src/index.js:1927-2103

[Andarist]

One thing I've learned from my past attempts at debugging React code is that I'm not that good at reasoning about such sublime code 🤣

Maybe @acdlite would be able to shed some light on this.

[acdlite]

The reason useState "just works" is because you can't update a component until after it has already hydrated

[Andarist]

nit: using an actual URL makes it easier for people to jump quickly to referenced files (if needed):

Suggested change

	// Matches logic in React's `shared/ExecutionEnvironment` file
	// Matches logic in React's `shared/ExecutionEnvironment` file
	// https://github.com/facebook/react/blob/5cccacd131242bdea2c2fe4b33fac50d2e3132b4/packages/shared/ExecutionEnvironment.js#L10-L14

[Andarist]

If I see correctly this is a leftover - this actually ain't used in this file atm.

[markerikson]

Sort of the other way around. I made this change speculating I might need to use this in connect somehow, but I haven't made any actual code changes to connect yet - that's why this is a WIP PR.

[Andarist]

If I understand correctly the serverState should be an immutable value, used only for hydration purposes. Might be worth skipping this here? Or putting it in a ref (that would be read by getServerState) to make this more explicit? Obviously, that would break the rules of hooks but IMHO it's sometimes worth it to represent some constraints etc

[acdlite]

A suggestion regarding connect:

What's nice about the connect API is that because it's a component factory/higher-order component, you can fork the implementation inside the factory instead of within the component's render body. You're already taking advantage of this by, for example, conditionally wrapping the ConnectFunction component with React.memo. But it looks like most the forking happens within ConnectFunction, inside the body of the component. Because of the hook rules, this means that a single component has to pay the cost of every possible hook that you might need to satisfy all the different connect API contracts.

One thing you could do to manage the complexity of these different variants is to lift the forked behavior out of the component and into the factory, like you already do with React.memo. Something like this:

// This code is way simplified, obviously, just trying to illustrate the basic idea
function connect(mapStateToProps, mapDispatchToProps, mergeProps) {
  return Component => {
    if (mapStateToProps == null) {
      // No selector provided
      // These don't need useSyncExternalStore at all.
      if (typeof mapDispatchToProps === 'function') {
        // Custom action creators
        return connectWithNoSelectorAndCustomActionCreators(Component, mapDispatchToProps);
      } else if (mapDispatchToProps != null) {
        // Basic action creators
        const actionCreators = mapDispatchToProps;
        return connectWithNoSelectorAndBasicActionCreators(Component, actionCreators);
      } else {
        // No action creators
        return connectNoOp(Component);
      }
    } else {
      // Selector provided
      if (mergeProps === null) {
        const selector = mapStateToProps;
        if (typeof mapDispatchToProps === 'function') {
          // Custom action creators
          return connectWithCustomActionCreators(Component, selector, mapDispatchToProps);
        } else if (mapDispatchToProps != null) {
          // Basic action creators
          const actionCreators = mapDispatchToProps;
          return connectWithBasicActionCreators(Component, selector, actionCreators);
        } else {
          // No action creators
          return connectWithNoActionCreators(Component, selector);
        }
      }
    }
    // ...and so on
  }
}

Then each branch can make different implementation trade offs, instead of needing to write a single component that handles every scenario. I think this would help with managing maintenance complexity. It would also be better for performance because each connected component is only paying the cost of the hooks that it needs. So for example, a connect with no mapStateToDispatch doesn't need a useSyncExternalStore.

[markerikson]

@acdlite yeah, as written right now, all that logic is offloaded into the "selector"-style logic over in the connect folder itself.

That's kinda what I was saying earlier - sure, it's possible to completely refactor / rewrite all that existing logic - it's "just" code! :) Question is how much effort it would take and how much benefit there would be.

Part of me is very tempted to just say: "look, connect is still here and continues to work as-is, but it's not going to get to take advantage of the 'proper' SSR/hydration support because it's too complex for us to deal with and it's a legacy API. If you want that to work right, switch to useSelector".

[acdlite]

That's kinda what I was saying earlier - sure, it's possible to completely refactor / rewrite all that existing logic - it's "just" code! :) Question is how much effort it would take and how much benefit there would be.

Maybe there's a compromise where we can make it work for the "simple" cases, then fall back to the current implementation for anything more complex. That might simplify the upgrade strategy/messaging for existing apps:

• All existing code just works
• If you use useSelector, it also works with Concurrent React features
• If you use connect without edge case APIs, it also works with Concurrent React features
• If you use connect with edge case APIs, you may or may not run into subtle concurrency issues. Consider migrating to useSelector if you care about Concurrent React compatibility.

[markerikson]

From Dan, a couple React 18 SSR demos that may be good to play with:

• https://codesandbox.io/s/kind-sammet-j56ro?file=/src/Html.js
• https://github.com/vercel/next-rsc-demo

[Ephem]

Nice work on this. connect is tricky indeed, but I think most of that doesn't relate to getServerSnapshot specifically, but supporting concurrency in general. 😅

To me it looks like this approach should get Redux to the first step described under "Hydrating Data" in Sebastians semi-new Library Upgrade Guide: <script> (e.g. SSR frameworks)
, that is, everything works when all state is emitted and available on the client before React hydration starts (just like today).

I think modifying Redux state with Suspense combined with streaming is where it's going to get really tricky, really fast, but also not something I'm sure Redux should support and either way a later problem. 😄

[Ephem]

I started thinking about ways to solve not having to pass in serverState explicitly here, and while possible, I realized the tradeoffs are not worth it (touching the redux-package for example). I think it makes sense to pass it in! 👍

[Ephem]

To support server snapshots here, connect would probably need to take a new serverState-option just like the Provider does. I think this is a niche use case, not sure if it warrants the complexity?

If this is not supported, passing store to connect could warn/error if serverState was passed to a Provider further up in the tree.

[Ephem]

If I understand things correctly (I might very well not!), the logic for handling prop changes etc shouldn't matter for the getServerSnapshot since it's called on hydration, so I think it could be roughly (not accounting for store as a prop etc):

() => childPropsSelector(contextValue.getServerState(), wrapperProps)

Unless uSES calls this again sometime after hydration, in which case you would need to snapshot the wrapperProps so they would stay the same, but I don't think this is the case.

As to how well the actualChildPropsSelector works overall after hydration, I couldn't say, it does seem like it could lead to bugs, but I think the above would at least be the equivalent way to do it for SSR?

[markerikson]

Yeah, I'm totally not going to bother letting that get passed to connected components as a prop. This is too complex already as it is :) Might be able to add that warning though.

I think you may be on the right track with the childPropsSelector suggestion!

[markerikson]

@Ephem Thanks for the feedback!

Could I ask you for a favor? The biggest thing I need atm is an example project that does Redux-based SSR, and ideally in conjunction with React 18 SSR, so that I have something to play with.

Dan linked me a couple React 18 SSR projects, which I pasted into a comment just above.

Could you either point me to a couple of Redux-based SSR projects so that I can smash the two together, or if you actually have time try to set up a Redux+React 18 project so that I can use it as a testbed?

I want to try to do this set of changes the right way, but I really don't have any meaningful React/Redux SSR experience to draw from. If I try to figure out how to set up all that stuff from scratch myself it's going to take a while before I even have something that I can test against :)

[Ephem]

I can't promise anything, but I'll try to hook you up with something basic at least! I've just started looking into React Query's React 18 SSR-support as well, so this is heavily on my mind now and I'll see if I can come up with some tests etc as well, though personal time is never in great supply. 😅

About those two examples, I don't think Redux should actually support either right now. Looks simple enough to rip out Suspense and progressive hydration from the first one and set up the base case I think should be supported thought!

---

Below this is just a look at the demos and why I don't think they should be supported now. Lets classify them:

https://codesandbox.io/s/kind-sammet-j56ro?file=/src/Html.js - Streaming rendering with Suspense data fetching and progressive hydration
https://github.com/vercel/next-rsc-demo - React Server Components

Neither Suspense data fetching nor RSCs are officially supported in the React 18 release and I think both are going to be hard to support fully in Redux. I'll try to write up why in more detail at some point, but it basically boils down to Redux being a single big store where any part of the app might behave differently based on any part of the store, and on hydration/getServerSnapshot all data needs to already be there. While it can support streaming, the app can not start hydrating before Redux has a full serverState so to speak, which means it gets tricky with Suspense where the store changes during rendering.

Or if you think about this great image from Shaundais fantastic talk, it's hard to imagine how Redux could currently hydrate these slices individually. RSCs are even trickier since they also happen on page navigation etc, so it's not really just hydration anymore.

[Ephem]

Oh and just to clarify, I used streaming and progressive hydration a bit sloppily, what I meant is hydrating while still streaming. The reason to implement getServerSnapshot at all is to support progressive hydration after all.

Say for example you only keep app state in Redux (maybe it's just the default reducer state + some state derived from cookies or something, known before rendering), but use Suspense for code splitting, implementing getServerSnapshot will let that app progressively hydrate as long as the initial Redux state is known when that hydration starts. There are a bunch of cases/variations where this is useful (even with server data in Redux, fetched ahead of time), it's just that they rely on the state being there for Redux before hydration starts. :)

[Ephem]

I wanted to get it done sooner than later (had a kid-free evening! 🎉), so I threw together something very quick and dirty from that first example: https://codesandbox.io/s/gifted-monad-npltr?file=/server/render.js

It's rough, but should showcase both how progressive hydration lets us click the button at the top before comments has hydrated, and that it will currently cause a hydration-mismatch for the comments section because a store value has updated. (Tearing itself is expected here, can't update a part of the app that hasn't hydrated yet)

I think this is the case Redux should aim to support for React 18. Data is "fetched" ahead of rendering on the server, and is available before hydration starts on the client, but progressive enhancement of Suspense-based code splitting should work without hydration mismatches, even when state updates in parents.

Hope it helps, didn't have time to comment it as much as I wanted, so do ask if things are unclear. 😄

Edit: I updated the sandbox slightly, so might not match the description above exactly, but still shows the same things.

[markerikson]

Sweet, thank you! May not be able to look at this for a bit due to other priorities, but I really appreciate your expertise on Redux+SSR in general and you putting this together specifically!

[markerikson]

Just rebased this against the latest type changes in master.

@Ephem : could I ask you to try converting that sandbox into a minimal standalone test file? I can play around with the sandbox as an actual project some (and I'll try to do that a bit now), but ultimately I'm going to want to have some kind of test that verifies this behavior works. Don't need it to pass yet :) just needs to do the equivalent store setup + rendering + hydration, and ideally fail based on React warning about hydration mismatch or something with connect, so that I can confirm that whatever changes I might make here fix the behavior.

[markerikson]

Well hey, early good news. I cloned the SSR example repo, added a store.dispatch(incrementCount()) on startup before initial render, and confirmed that I get a hydration warning with both 7.2.6 and 8.0.0-beta.1.

I then used yalc to add a build from this branch, updated <App> to pass through <Provider serverState={initialState}>, and confirmed that there is no hydration warning!

So that looks like useSelector works okay. Now to play around with connect and see what I can come up with...

[markerikson]

Aaaand....

applying @Ephem 's suggestion to connect seems to work!

        actualChildProps = useSyncExternalStore(
          subscribeForReact,
          actualChildPropsSelector,
          // Use the server state if it's available
          getServerState
            ? () => childPropsSelector(getServerState(), wrapperProps)
            : actualChildPropsSelector

I added a second "count" button to the demo app that uses connect instead for the store state, verified that I see a hydration warning with the build from the PR, then made that change locally and verified that I do not see a hydration warning.

This really needs more extensive testing, but given that I also see I screwed up the beta by adding some things into deps instead of devDeps, I think I'm going to go ahead and merge these changes now, and put out beta.2. We can add more tests in a follow-up PR.

[markerikson]

told you I really needed a viable demo to compare against :)

[Andarist]

q: this component still relies on a custom Subscription and custom notification propagation etc. Doesn't useSyncExternalStore solve the ordering issue?

[markerikson]

Nope, not at all. uSES still creates store subscriptions same as always, and React still runs effects bottom-to-top on mount. So, children still subscribe before parents, etc, and we still have to do work on our side to enforce the "top-down updates" requirement for connect.

The only thing in React that has ever helped with that was when we switched v6 to read state updates from context. It worked.... which is why it was so frustrating that it also turned out to be the worst case for performance updates.

[Andarist]

we still have to do work on our side to enforce the "top-down updates" requirement for connect.

Isn't the problem relevant to useSelector too? Is React subscribing to the "external store" (Redux) with uSES still in the order that makes shenanigans required? Is the zombie children problem still relevant with uSES? Or is the problem somewhat fixed with automatic batching and top-down rerender that should happen with it? As automatic batching should end up rerendering things in top-down order, right?

[markerikson]

Yes, the problem is entirely relevant to useSelector, and there's absolutely nothing we can do to fix it - because hooks can't render context providers, so we can't enforce top-down behavior. That's why I documented all the "zombie child" concepts in the hooks API page in the first place:

https://react-redux.js.org/api/hooks#stale-props-and-zombie-children

And no, batching still has nothing to do with this problem, because it's the subscription callbacks themselves that are the problem, and those run outside of React when the store is updated.

The hacky-but-valid workaround that both useSelector and uSES take is catching errors in a selector when subscribers are notified, forcing a re-render anyway, re-running the selector during the render, and seeing if it explodes again or succeeds.

That workaround is good enough that it does actually mostly mitigate the issue. But the root cause of the problem still exists - components can end up subscribing to the store in an order where a child is notified before its parent, and if the child depends on using props in its selector, those props can be stale or the child may be reading state right before its parent is about to stop rendering the child.

[Andarist]

Ok, I understand how automatic batching is not the remedy for the problem.

That workaround is good enough that it does actually mostly mitigate the issue. But the root cause of the problem still exists - components can end up subscribing to the store in an order where a child is notified before its parent, and if the child depends on using props in its selector, those props can be stale or the child may be reading state right before its parent is about to stop rendering the child.

I must say that I'm somewhat disappointed with the uSES API then. I was under the impression that this problem would actually be solved by this API. This problem is affecting all external stores and thus the implementation that would guard users against this problem is still very complicated - so marketing uSES as a tool solving external store problems is somewhat misleading. I think that what it solves was way easier to solve in the userland (maybe apart from allowing inline selectors, as that was super tricky) than the zombie children problem - solving this problem at the React level would be the bigger "win" for the library authors.

@acdlite is solving this problem out of the scope of uSES? How are other external stores supposed to handle those situations? Should we use the hacky solution outlined above by @markerikson?

[markerikson]

FWIW uSES already implements that "hacky solution" :) and specifically inspired by useSelector doing it first.

Really the problem is a mismatch in data shapes: a linear array of subscriber callbacks executed in order, vs a tree of components where A) mount effects are executed bottom-to-top, and B) components can be added and removed over time.

If there were some way to actually correlate "adding a subscriber" with "actual position in the React component tree", then it would be theoretically possible to build a smarter notification system that actually notifies subscribers in tree parent->child order. But there's no way to know where a given component is in the tree.

[Andarist]

If there were some way to actually correlate "adding a subscriber" with "actual position in the React component tree", then it would be theoretically possible to build a smarter notification system that actually notifies subscribers in tree parent->child order. But there's no way to know where a given component is in the tree.

I would put the asterisk here that it's no way to know that in the userland - that's why I think this is the problem worth solving in the React itself (in the uSES).

FWIW uSES already implements that "hacky solution" :) and specifically inspired by useSelector doing it first.

Hm, so I'm a little bit confused right now. I wonder if I understand the whole zombie children problem right now - I think I understand the problem but I might not understand how the problem is solved in React Redux today.

So to get us on the same page. This is what I think right now:

1. zombie children problem is caused~ by subscriber callbacks being called "out of order" because a child's callback can be called before the parent's callback - this leads to errors being thrown when items in the store are deleted during the latest dispatch~
2. the error is captured by React Redux and a new rerender is being scheduled
3. that new rerender is already executed when props are already updated and thus the selector should no longer throw or a rerender for that child might not happen at all because that component could have been unmounted by the render that has thrown (unmounted by its parent rerendering with the selected state that already doesn't contain the deleted item)
4. so in a sense - the whole zombie children problem is already "solved" by capturing errors and rerendering, the point is that users of React Redux don't have to deal with this. They might occasionally observe weird-ish things when debugging, but that's acceptable.
5. however, there is still the cost of the child being rendered at all as that could have been prevented if subscriptions would merely mark components as "dirty" and the state would actually be selected in render - with such a solution the child's subscriber wouldn't select anything that could throw (if only this would be batched and performed in the top-down order). Or if the item wasn't completely removed and the error is caused by partial lack of information etc then we get 2 renders there instead of one.

Is this reasoning correct? I think that I see where the use-sync-external-store/shim implements the try/catch dance and that looks good. It means that libraries using that won't have to deal with this problem at all because that's covered there. I wonder though - if the implementation of the "native" useSyncExternalStore the same or is the issue mitigated there because things can actually be executed in the "correct" order in React 18?

[markerikson]

Yep, that's all correct. (fwiw I don't think the "cost" thing in step 5 is really a meaningful issue.)

You can see that the actual useSyncExternalStore does the same 'rerender if error" behavior here:

https://github.com/facebook/react/blob/3dc41d8a2590768a6ac906cd1f4c11ca00417eee/packages/react-reconciler/src/ReactFiberHooks.new.js#L1482-L1491

[Andarist]

Yep, that's all correct. (fwiw I don't think the "cost" thing in step 5 is really a meaningful issue.)

I agree that this could have been somewhat exaggerated on my side. If this is only affecting subscribers and not rerenders then I guess this is somewhat OK (I assume that the inner component might in fact not even rerender, even if it throws, if it gets "unmounted" by the parent). However, I still find it a little bit strange that React 18 doesn't try to mitigate this all together in a smarter way - but dunno, maybe the smarter way would have some perf implications for the whole thing.

[Ephem]

Nice work! 👏 Glad the sandbox helped and yeah, I plan to add that and more tests, though I have some other things to look at first. 😀

There might be some more nuance here, I'm still thinking and exploring, I'll get back with any findings.