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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Dec 22, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bc4569f
Add tentative SSR support for useSelector
markerikson Nov 2, 2021
63f626b
Comment areas of concern for `connect` + `uSES`
markerikson Nov 2, 2021
ff4f9d1
Update React deps to 18 RC.0 and fix wrongly added devDeps
markerikson Dec 22, 2021
ed8fdc8
Actually use server state for hydration in `connect` if available
markerikson Dec 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
"coverage": "codecov"
},
"peerDependencies": {
"react": "^18.0.0-beta"
"react": "^18.0.0-rc"
},
"peerDependenciesMeta": {
"react-dom": {
Expand All @@ -52,15 +52,11 @@
},
"dependencies": {
"@babel/runtime": "^7.12.1",
"@testing-library/react-12": "npm:@testing-library/react@^12",
"@types/hoist-non-react-statics": "^3.3.1",
"@types/use-sync-external-store": "^0.0.3",
"hoist-non-react-statics": "^3.3.2",
"react-17": "npm:react@^17",
"react-dom-17": "npm:react-dom@^17",
"react-is": "^18.0.0-beta-fdc1d617a-20211118",
"react-test-renderer-17": "npm:react-test-renderer@^17",
"use-sync-external-store": "1.0.0-beta-fdc1d617a-20211118"
"react-is": "^18.0.0-rc.0",
"use-sync-external-store": "^1.0.0-rc.0"
},
"devDependencies": {
"@babel/cli": "^7.12.1",
Expand All @@ -81,6 +77,7 @@
"@testing-library/jest-dom": "^5.11.5",
"@testing-library/jest-native": "^3.4.3",
"@testing-library/react": "13.0.0-alpha.4",
"@testing-library/react-12": "npm:@testing-library/react@^12",
"@testing-library/react-hooks": "^3.4.2",
"@testing-library/react-native": "^7.1.0",
"@types/object-assign": "^4.0.30",
Expand All @@ -104,9 +101,12 @@
"jest": "^26.6.1",
"prettier": "^2.1.2",
"react": "18.0.0-beta-fdc1d617a-20211118",
"react-17": "npm:react@^17",
"react-dom": "18.0.0-beta-fdc1d617a-20211118",
"react-dom-17": "npm:react-dom@^17",
"react-native": "^0.64.1",
"react-test-renderer": "18.0.0-beta-fdc1d617a-20211118",
"react-test-renderer-17": "npm:react-test-renderer@^17",
"redux": "^4.0.5",
"rimraf": "^3.0.2",
"rollup": "^2.32.1",
Expand Down
1 change: 1 addition & 0 deletions src/components/Context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export interface ReactReduxContextValue<
> {
store: Store<SS, A>
subscription: Subscription
getServerState?: () => SS
}

export const ReactReduxContext =
Expand Down
16 changes: 12 additions & 4 deletions src/components/Provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,32 +4,40 @@ import { createSubscription } from '../utils/Subscription'
import { useIsomorphicLayoutEffect } from '../utils/useIsomorphicLayoutEffect'
import { Action, AnyAction, Store } from 'redux'

export interface ProviderProps<A extends Action = AnyAction> {
export interface ProviderProps<A extends Action = AnyAction, S = any> {
/**
* The single Redux store in your application.
*/
store: Store<any, A>
store: Store<S, A>

/**
* An optional server state snapshot. Will be used during initial hydration render if available, to ensure that the UI output is consistent with the HTML generated on the server.
*/
serverState?: S

/**
* Optional context to be used internally in react-redux. Use React.createContext() to create a context to be used.
* If this is used, you'll need to customize `connect` by supplying the same context provided to the Provider.
* Initial value doesn't matter, as it is overwritten with the internal state of Provider.
*/
context?: Context<ReactReduxContextValue<any, A>>
context?: Context<ReactReduxContextValue<S, A>>
children: ReactNode
}

function Provider<A extends Action = AnyAction>({
store,
context,
children,
serverState,
}: ProviderProps<A>) {
const contextValue = useMemo(() => {
const subscription = createSubscription(store)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@markerikson markerikson Dec 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@markerikson markerikson Dec 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return {
store,
subscription,
getServerState: serverState ? () => serverState : undefined,
}
}, [store])
}, [store, serverState])
Copy link
Contributor

Choose a reason for hiding this comment

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

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


const previousState = useMemo(() => store.getState(), [store])

Expand Down
19 changes: 16 additions & 3 deletions src/components/connect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ import defaultMapStateToPropsFactories from '../connect/mapStateToProps'
import defaultMergePropsFactories from '../connect/mergeProps'

import { createSubscription, Subscription } from '../utils/Subscription'
import { useIsomorphicLayoutEffect } from '../utils/useIsomorphicLayoutEffect'
import {
useIsomorphicLayoutEffect,
canUseDOM,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

} from '../utils/useIsomorphicLayoutEffect'
import shallowEqual from '../utils/shallowEqual'

import {
Expand Down Expand Up @@ -124,6 +127,7 @@ function subscribeUpdates(
return
}

// TODO We're currently calling getState ourselves here, rather than letting `uSES` do it
const latestStoreState = store.getState()

let newChildProps, error
Expand Down Expand Up @@ -157,6 +161,7 @@ function subscribeUpdates(
childPropsFromStoreUpdate.current = newChildProps
renderIsScheduled.current = true

// TODO This is hacky and not how `uSES` is meant to be used
// Trigger the React `useSyncExternalStore` subscriber
additionalSubscribeListener()
}
Expand Down Expand Up @@ -597,6 +602,10 @@ function connect<
? props.store!
: contextValue!.store

const getServerState = didStoreComeFromContext
? contextValue.getServerState
: store.getState

const childPropsSelector = useMemo(() => {
// The child props selector needs the store reference as an input.
// Re-create this selector whenever the store changes.
Expand Down Expand Up @@ -724,10 +733,14 @@ function connect<

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

actualChildProps = useSyncExternalStore(
// TODO We're passing through a big wrapper that does a bunch of extra side effects besides subscribing
subscribeForReact,
// TODO This is incredibly hacky. We've already processed the store update and calculated new child props,
// TODO and we're just passing that through so it triggers a re-render for us rather than relying on `uSES`.
actualChildPropsSelector,
// TODO Need a real getServerSnapshot here
actualChildPropsSelector
getServerState
? () => childPropsSelector(getServerState(), wrapperProps)
: actualChildPropsSelector
)
} catch (err) {
if (latestSubscriptionCallbackError.current) {
Expand Down
4 changes: 2 additions & 2 deletions src/hooks/useSelector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,13 @@ export function createSelectorHook(
}
}

const { store } = useReduxContext()!
const { store, getServerState } = useReduxContext()!

const selectedState = useSyncExternalStoreWithSelector(
store.subscribe,
store.getState,
// TODO Need a server-side snapshot here
store.getState,
getServerState || store.getState,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

selector,
equalityFn
)
Expand Down
8 changes: 5 additions & 3 deletions src/utils/useIsomorphicLayoutEffect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ import { useEffect, useLayoutEffect } from 'react'
// is created synchronously, otherwise a store update may occur before the
// subscription is created and an inconsistent state may be observed

export const useIsomorphicLayoutEffect =
// Matches logic in React's `shared/ExecutionEnvironment` file
Copy link
Contributor

Choose a reason for hiding this comment

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

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

export const canUseDOM = !!(
typeof window !== 'undefined' &&
typeof window.document !== 'undefined' &&
typeof window.document.createElement !== 'undefined'
? useLayoutEffect
: useEffect
)

export const useIsomorphicLayoutEffect = canUseDOM ? useLayoutEffect : useEffect
25 changes: 16 additions & 9 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8837,7 +8837,7 @@ __metadata:
languageName: node
linkType: hard

"react-is@npm:18.0.0-beta-fdc1d617a-20211118, react-is@npm:^18.0.0-beta-fdc1d617a-20211118":
"react-is@npm:18.0.0-beta-fdc1d617a-20211118":
version: 18.0.0-beta-fdc1d617a-20211118
resolution: "react-is@npm:18.0.0-beta-fdc1d617a-20211118"
checksum: 402e28c80019e9deaee919c4373606736ea36142d1d1e09e0cab89d52e8bd647f1176a383987cca5c4e83bfee33bba8091ab347363ea4bdd4f9c3a707812c8b9
Expand All @@ -8858,6 +8858,13 @@ __metadata:
languageName: node
linkType: hard

"react-is@npm:^18.0.0-rc.0":
version: 18.0.0-rc.0-next-f2a59df48-20211208
resolution: "react-is@npm:18.0.0-rc.0-next-f2a59df48-20211208"
checksum: e0f8b1c8ef759153072b7508f8832164651aa855c9c939d607090899d9d522c68ad12ec9630f34a56c46330542356207dfc09d744b8ffe2eccc45a83f296fcae
languageName: node
linkType: hard

"react-native-codegen@npm:^0.0.6":
version: 0.0.6
resolution: "react-native-codegen@npm:0.0.6"
Expand Down Expand Up @@ -8966,7 +8973,7 @@ __metadata:
react-17: "npm:react@^17"
react-dom: 18.0.0-beta-fdc1d617a-20211118
react-dom-17: "npm:react-dom@^17"
react-is: ^18.0.0-beta-fdc1d617a-20211118
react-is: ^18.0.0-rc.0
react-native: ^0.64.1
react-test-renderer: 18.0.0-beta-fdc1d617a-20211118
react-test-renderer-17: "npm:react-test-renderer@^17"
Expand All @@ -8976,9 +8983,9 @@ __metadata:
rollup-plugin-terser: ^7.0.2
ts-jest: 26.5.6
typescript: ^4.3.4
use-sync-external-store: 1.0.0-beta-fdc1d617a-20211118
use-sync-external-store: ^1.0.0-rc.0
peerDependencies:
react: ^18.0.0-beta
react: ^18.0.0-rc
peerDependenciesMeta:
react-dom:
optional: true
Expand Down Expand Up @@ -10845,12 +10852,12 @@ __metadata:
languageName: node
linkType: hard

"use-sync-external-store@npm:1.0.0-beta-fdc1d617a-20211118":
version: 1.0.0-beta-fdc1d617a-20211118
resolution: "use-sync-external-store@npm:1.0.0-beta-fdc1d617a-20211118"
"use-sync-external-store@npm:^1.0.0-rc.0":
version: 1.0.0-rc.0-next-f2a59df48-20211208
resolution: "use-sync-external-store@npm:1.0.0-rc.0-next-f2a59df48-20211208"
peerDependencies:
react: 18.0.0-beta-fdc1d617a-20211118
checksum: bb52d87a886731595163a3b6a07b671f9ea92ca6de35791f32ce186e2fea0f2989088f3dacfea3dff258e866e767ff1e87d18494e436523e67b17e59051a22e0
react: 18.0.0-rc.0-next-f2a59df48-20211208
checksum: 772bf9fe4b47f9cf21969cc839e21e11e2fbad63e4992c75402fabed7048f5afb559bee289c9a0c3b4bad7c1c5724f6cb63c5499ef7f249c395e86233102bccb
languageName: node
linkType: hard

Expand Down