Initial experimental React 18 compat prototyping

[markerikson]

This PR:

• Updates our React devDeps to 18.experimental versions, along with React Testing Library's PR build from feat: Use concurrent React when available testing-library/react-testing-library#937
• Updates our TS config to use the types for those React builds
• Fixes almost all of the failing tests, which were primarily due to state updates being triggered outside of act()
• Actually swaps useSelector over to use useSyncExternalStore instead of our own subscription logic
• Also updates connect to use useSyncExternalStore, albeit in a kinda sketchy way
• Rips out the pure option for connect

@dai-shi has a proof-of-concept PR at pmndrs/zustand#550 that will likely be useful as a starting point.

Status

At this point I see 3 test failures. Two look like they're related to tests for "avoid setting state on unmount", and the other looks like it's a batching-related issue. Need to investigate those further.

Note that this so far involves no library code changes whatsoever. Once I get the current tests updated to be passing based on expected behavior with React 18 overall, the next step is to try switching our logic over to use useSyncExternalState per facebook/react#22211 , and see what happens.

I've just commented out / tweaked those three test failures for now so I can move on. I'll review them more later.

Update 1

I just successfully got useSelector migrated over to use useSyncExternalStore! I'm seeing one legit test failure atm - uSES is not correctly preserving the previous selected result when you give it a new selector reference, because of how it memoizes things.

I wrote some notes in facebook/react#22211 , and I'll paste those here:

• React-Redux currently catches errors thrown from selectors inside of subscriptions, saves them in a ref, and forces a re-render. If the selector runs clean while rendering, no problem. If it throws again, we attach the original error's message to the new error just in case it was two different things that happened. However, uSES always swallows errors in checkIfSnapshotChanged, so we can no longer check the original error ourselves. I was able to work around this by having useSelector wrap the original selector with one that does a try/catch and saves errors to a ref, but wow is that hacky.
• Is the "not a valid React 18 version" check accidentally inverted? It currently says if (React.startTransition !== undefined) {, which seems wrong. I'm currently running 18.0.0-alpha-1314299c7-20210901, which most definitely has a startTransition function defined, and I am seeing that warning printed.
• Our test for "checks for updates when subscribing and re-renders if different" is seeing a third render occur, when it used to only see two, and I can't figure out what's causing that extra render to happen (especially since renders 2 and 3 are seeing the same selected state). My vague hypothesis is that it may have something to do with the test dispatching an action in a useLayoutEffect in a child, while the subscription happens in a useEffect in the hook in the parent?
• The way that useSyncExternalStoreExtra's memoization check is implemented means that if you get a new selector function, there's no way for it to be equal to the prior result and reuse it, because all of the memoization variables are internal to the useMemo hook. Our v7 useSelector does this behavior by keeping around the last selected value in a useRef. We have a test for that in our codebase, and it's failing atm, and I'm pretty sure this is a "correct" failure based on what I'm seeing in the uSES implementation.

Update 2

I've just gotten connect updated as well, and I've also ripped out the pure option.

I'm not convinced that the way I integrated useSyncExternalStore into connect is at all "correct". connect is still doing all the actual child props derivation logic. It's really just deferring to uSES for the "trigger a re-render" step.

All of the "impure behavior" tests were failing with this approach. After thinking about it, this option seemed ripe for removal. It only existed because early React ecosystem usage sometimes still relied on external mutable sources that would be different every time the component re-rendered, so we always had to re-run mapState and the props merging logic. In today's world, that's no longer the case - external mutability is no longer a thing that shows up in codebases, and at this point if you're still doing that it's a bug.

I did run a SourceGraph search to try to find any places where people were actually using {pure: false}. Best I could come up with was https://sourcegraph.com/search?q=context:global+lang:JavaScript+connect%28.%2B%2C+.%2B%2C+.%2B%2C+%7B.*pure%5Cs%3F:%5Cs%3Ffalse.*%7D%29&patternType=regexp , which only had 59 hits in 1.2M repos. Of those 59 hits, half were FlowType typedefs. So, this seems safe to kill off, and it simplifies several other bits of the remaining codebase anyway.

At this point, there's two tests failing, and ironically they're both bugs in uSES itself:

• It's not correctly memoizing and reusing the last returned value if it gets a new selector reference
• It's printing a "no useLayoutEffect in SSR" warning

Now that I think about it, I do have some tests for useSelector that I'd temporarily tweaked and gotten to "pass" by changing the assertions just to let me keep going. I need to review those further.

[netlify]

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

🔨 Explore the source changes: 7c2b73f

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

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

[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 7c2b73f:

Sandbox	Source
Vanilla	Configuration
Vanilla Typescript	Configuration

[markerikson]

Hmm. Not sure why the tests aren't running. Created this as a "draft" initially and thought that might be why, but doesn't look like they're running after changing it to a normal PR either.

[timdorr]

If you rebase off master, it should have a button for running tests now.

[github-actions]

Size Change: +3.38 kB (15%) ⚠️

Total Size: 22 kB

Filename	Size	Change
dist/react-redux.js	17.3 kB	+3.12 kB (18%)	⚠️
dist/react-redux.min.js	4.73 kB	+262 B (5%)	🔍

_{compressed-size-action}

[eps1lon]

Is the "not a valid React 18 version" check accidentally inverted? It currently says if (React.startTransition !== undefined) {, which seems wrong. I'm currently running 18.0.0-alpha-1314299c7-20210901, which most definitely has a startTransition function defined, and I am seeing that warning printed.

I think this warning is intended since the shim of useSyncExternalStore does not support React 18. The warning should disappear in React 17 or once facebook/react#22239 lands.

[acdlite]

It's not correctly memoizing and reusing the last returned value if it gets a new selector reference

This is an intentional part of the design of useSyncExternalStore — we always call getSnapshot during render, because it might have been changed by a parent. That's how we avoid zombie props.

The "extra" version of the API supports skipping the additional call during render... but only if you pass the same selector. So it has to be stable/memoized. (The difference compared to useMutableSource is that it won't de-opt to synchronous mode if it renders during a transition, and it won't resubscribe.)

Alternatively, if you'd rather use the zombie props (to preserve Redux's existing behavior, I'm guessing?), you might as well skip the "extra" version of the API and track the latest selector on a ref like you currently do, since our selector implementation won't provide any additional benefit over the lower-level API.

[acdlite]

Aside from preserving existing behavior, though, I'd say that for the majority of cases it's fine not to memoize the selector. Especially if they don't pass a custom isEqual argument. Most selectors aren't that computationally expensive.

For those cases where the selectors are expensive, there's a straightforward solution: wrap it in useCallback. You can tell users to think of it as an optimization, similar to other scenarios in React where you'd use useMemo, memo, or PureComponent. But you wouldn't want to do this prematurely everywhere, because usually the impact of running the selector again is negligible compared to the weight of another hook.

This is going to be our advice for useSyncExternalStore in general.

[markerikson]

@acdlite I'm not sure I explained the "memoized value vs new selector ref" issue enough here.

When a new selector reference gets passed into useSelector, yes, we do need to call the new selector because it's entirely possible that it could return something completely different than the old one did. (Granted, 99.9% of the time it's the exact some function code-wise, just inlined.) However, it's also possible (and even likely) that the value returned from the new selector passes the equality check vs the last returned value. In that case, we want to still reuse the old memoized value and return it, thus avoiding an extra unnecessary re-render.

What I'm seeing with the uSESExtra implementation atm is that because of how the memoization is implemented via a closure, it's impossible to get a new selector ref and still refer to the old result value to return it.

[acdlite]

However, it's also possible (and even likely) that the value returned from the new selector passes the equality check vs the last returned value. In that case, we want to still reuse the old memoized value and return it, thus avoiding an extra unnecessary re-render.

Ah I see, ok I think it makes sense to support that

[acdlite]

What would happen if you stripped down the implementation as much as possible? What features is React missing in the current proposal? (Other than not bailing out on equal selections when the selector changes, as discussed above, which we'll add.)

In other words, in what ways would the following implementation be insufficient:

function useSelector(selector, isEqual) {
  const store = useReduxStore();
  return useSyncExternalStoreExtra(
    store.subscribe,
    store.getState,
    selector,
    isEqual,
  );
}

Our goal with useSyncExternalStore is to upstream a lot of the complexity you're currently having to deal with in userspace, so whatever we're missing we'd love to hear about it.

I think the main extra things you currently have are:

• a subscription manager that's aware of the tree structure
• an error replay mechanism if a selector throws

My understanding is that both of these exist to account for the zombie children scenario. However, useSyncExternalStore is designed to avoid that, so I wonder which of your tests would break if you removed them and tried relying only on useSES?

[acdlite]

an error replay mechanism if a selector throws

I think we mentioned this previously, but React actually uses this exact same mechanism for all errors, which we leverage in useSyncExternalStore. So I'm hoping you can rely on that instead of implementing it again yourself.

[acdlite]

However, it's also possible (and even likely) that the value returned from the new selector passes the equality check vs the last returned value. In that case, we want to still reuse the old memoized value and return it, thus avoiding an extra unnecessary re-render.

So I remembered why I didn't implement it this way: React doesn't need the reference to be the same to skip re-rendering.

The way bailouts in React work is that, if a component re-renders but none of the state or props have changed, we assume the children are equal even if they fail an equality check.

So even if useSyncExternalStoreExtra returns a new reference, if no other state in the component has changed, then React will still bail out.

If you have a test that asserts the references are equal, then it will obviously fail, but if your test instead asserts that the children don't re-render, then it should pass.

If you find this still doesn't work as expected, could you point me to a failing test so I can take a look?

EDIT: Though I suppose it's still useful to return a memoized value even if the component does re-render, since it could preserve downstream memoizations. Ok disregard this comment, I'll go ahead and add this behavior.

[markerikson]

Sounds good. And yeah, the currently failing test is at https://github.com/reduxjs/react-redux/pull/1808/files#diff-be963b0f28d22eab25f8f42209afbe9a4f5d16696f016853c310c7832303cdb5R621-R623 :

        it('reuse latest selected state on selector re-run', () => {
          const alwaysEqual = () => true
          const Comp = () => {
            // triggers render on store change
            useNormalSelector((s) => s.count)
            const array = useSelector(() => [1, 2, 3], alwaysEqual)
            renderedItems.push(array)
            useLayoutEffect(() => {
              renderedItems.push(array)
            })
            return <div />
          }

          rtl.render(
            <ProviderMock store={normalStore}>
              <Comp />
            </ProviderMock>
          )

          expect(renderedItems.length).toBe(1)

          normalStore.dispatch({ type: '' })
          act(() => {
            normalStore.dispatch({ type: '' })
          })

          expect(renderedItems.length).toBe(2)
          // TODO This is failing, and correctly so. `uSES` drops the memoized value if it gets a new selector.
          expect(renderedItems[0]).toBe(renderedItems[1])
        })

[markerikson]

@acdlite I'll have to circle back to your other questions about "missing scenarios" later when I have more time to think about it.

You can see in this PR that I did some very stupid hacky things to get connect to continue working as-is - mostly the need to trigger those cascading subscriptions inside of the subscription listeners. Not sure there's anything applicable for uSES out of that, though.

For the error handling, useSelector currently tries to keep the original error from the subscriber around for reference, so that if it happens again while rendering, we can say "error Y may be related to previous error X". Tbh I'm not sure how fully useful that is, and falling back to uSES's own error handling may be acceptable here.

One other sorta-related train of thought. connect has long had an areStatesEqual option that bails out if the root state is considered the same. For some reason we never got around to adding that to useSelector. We recently did that in 7.2.5 with a simple if (prevState === currentState) return, and a user filed #1813 in response complaining that their app was breaking. Turns out it's some high-powered finance app that actually does really mutate the Redux state (!).

I mentioned that we could at least consider adding a similar areStatesEqual option to useSelector. Not sure if we'll actually end up doing that, but I don't think that would be feasible with uSES because it does its own is(prevState, currentState) check inside. I'm not saying that you should change that - just that it's a hypothetical scenario that uSES doesn't support.

[acdlite]

Since this is not idiomatic Redux and you don't already support it, you could add that as a separate API?

[acdlite]

Ok I fixed the memoization issue: facebook/react#22307.

Available in the experimental release channel, starting with use-sync-external-store@0.0.0-experimental-fd5e01c2e-20210913

Lemme know if it works as expected

[markerikson]

The idea of an areStatesEqual comparison does have application beyond "we're being bad and mutating the state". For example, if you know that you're only ever reading from state.x, you could supply (prevState, newState) => prevState.x !== newState.x and avoid any execution of the selectors for this component until you know that state.x has changed. So, sort of a pre-optimization that could skip the "run the selector" stage entirely.

connect has had that option since v5. I'm not sold on adding it, but since the question did just come up in that other thread, it was just on my mind as a thing that I don't know if we could do with uSES as currently implemented. Maybe we could, if we did some getSnapshot shenanigans, but unlikely it would be worth the effort (especially given that we've gotten along without that option thus far).

[markerikson]

@acdlite Just updated all our React devDep packages locally to 0.0.0-experimental-fd5e01c2e-20210913. Looks like that does pass the "return the last memoized value" test now.

I only see one other current test failure, which is the "no warnings from useLayoutEffect in SSR" test :)

but I do have a couple other render-count related tests I need to go back and review a bit.

Also, as I commented over in facebook/react#22307, the shim isn't looking for the unstable_ prefix, so as currently published it can never use the built-in version of uSES.

[markerikson]

Huh. @acdlite , I clearly have issues understanding how the whole dispatcher thing works :)

unstable_useSyncExternalStore does not appear to be exported by react-dom@0.0.0-experimental-fd5e01ce-20210913. However, it seems like it is made available on the dispatcher, and if I hand-edit use-sync-external-store.development.js to refer to builtInAPI = React.unstable_useSyncExternalStore, and I add a logging line to the uSES function in react.development.js, I can confirm that the built-in API is being called.

In fact, when I do that, I actually see better test results in a way.

When I was doing the first work on this PR earlier, I saw three of our tests that do render counts in assertions start failing because of an extra render pass happening, and I couldn't figure out what was causing it.

If I do these hand-edits to force the use of the built-in uSES, I'm now seeing those tests revert back to the original (and expected) number of renders. Here's the "failures" with the built-in version that show the reversion to the original behavior:

  ● React › hooks › useSelector › lifecycle interactions › notices store updates between render and store subscription effect

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 0

      Array [
        0,
        1,
    -   1,
      ]

      238 |           // 2) Render due to dispatch, still sync in the initial render's commit phase
      239 |           // TODO 3) ??
    > 240 |           expect(renderedItems).toEqual([0, 1, 1])
          |                                 ^
      241 |         })
      242 |       })
      243 |

      at Object.<anonymous> (test/hooks/useSelector.spec.tsx:240:33)

  ● React › hooks › useSelector › performance optimizations and bail-outs › calls selector twice once on mount when state changes during render

    expect(received).toEqual(expected) // deep equality

    Expected: 3
    Received: 2

      427 |           // TODO As with "notice store updates" above, we're now getting _3_ renders here
      428 |           // expect(renderedItems.length).toEqual(2)
    > 429 |           expect(renderedItems.length).toEqual(3)
          |                                        ^
      430 |         })

So, it looks like there's a slight difference in behavior between the shim and the built-in implementation, which is not terribly surprising.

This is interesting. We should want to have our tests run against the built-in version, but I can't just hand-edit the shim file to make it work the way I want to as a real solution.

Also not sure how to handle the difference in render counts here. Will have to think about this a bit.

The one other bit of weirdness I'm seeing is with @testing-library/react-hooks. With my hand edits to use-sync-external-store.development.js, the two tests we have that actually call renderHooks() are exploding because they can't find the right function:

  ● React › hooks › useSelector › core subscription behavior › selects the state and renders the component when the store updates

    TypeError: dispatcher.useSyncExternalStore is not a function

      36 |   }, [store, contextSub])
      37 |
    > 38 |   return useSyncExternalStoreExtra(
         |          ^
      39 |     subscribe,
      40 |     store.getState,
      41 |     selector,

      at Object.useSyncExternalStore (node_modules/react/cjs/react.development.js:1626:21)
      at Object.useSyncExternalStoreExtra (node_modules/use-sync-external-store/cjs/use-sync-external-store-extra.development.js:112:36)
      at useSelectorWithStoreAndSubscription (src/hooks/useSelector.ts:38:10)
      at useSelector (src/hooks/useSelector.ts:84:27)
      at react_hooks_1.renderHook.wrapper.react_1.default.createElement (test/hooks/useSelector.spec.tsx:68:47)
      at TestHook (node_modules/@testing-library/react-hooks/lib/pure.js:51:14)
      at renderWithHooks (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:6080:18)

I can only assume there's some shenanigans going on with import timing and Jest modules somehow? Perhaps it also has to do with React Hooks Testing Library not being updated yet.

If I change those two tests over to be normal function components + rtl.render(), instead of RHTL's renderHooks(), those two tests pass nicely with the built-in API.

[acdlite]

I think the issue is just that useSyncExternalStore is still prefixed, even in the experimental channel, so the use-sync-external-store package doesn't pick up the native version yet. It always uses the shim.

The reason that matters is because the tests are running in a concurrent root (via React Testing Library*), but the shim is not designed to work in concurrent roots. It only works in legacy roots, where updates are always sync.

The idea is that the shim will always pick the native version in React 18, and always the shim in 17 and below, because useSyncExternalStore doesn't exist in anything before 18.

But the problem is, right now, the native API is prefixed with unstable_. So the package doesn't detect it, and falls back to the shim. That's why you get the warning, as @eps1lon explained here: #1808 (comment)

Until the useSyncExternalStore API is finalized, what I'll do is update the shim to import the unstable version. I should have done that from the beginning, didn't consider this in-between early testing phase.

In the meantime, you can continue your testing by downgrading React and React DOM back to 17.

(* I didn't realize React testing library automatically uses createRoot when it's available. We should ask them (and by "them" I guess I mean @eps1lon) not do that, since there are enough subtle batching changes that it's likely to break some existing tests if they all get automatically switched over when you upgrade React. But this is a perfect example of why we prefix things — people add feature testing for unstable APIs and then when we ship the stable it has the potential break their users. See also: remix-run/react-router#7308. OTOH if React Testing Library users are already using act everywhere then maybe it'll just work? Doesn't seem like that's ​the case, though.)

[acdlite]

(I should have done that from the beginning, didn't consider this in-between early testing phase.)

Well, I kind of did — that's what the warning was for :D

[acdlite]

Ok the prefix issue is fixed now, starting with 0.0.0-experimental-806aaa2e2-20210913

[acdlite]

I can only assume there's some shenanigans going on with import timing and Jest modules somehow? Perhaps it also has to do with React Hooks Testing Library not being updated yet.

It looks like you updated the react and react-dom packages, but not react-test-renderer. The API is imported from the react package (which is why use-sync-external-store attempts to use it) but the renderer doesn't implement it. If you change the versions to match (either both 17 or both 18) then it will work.

[eps1lon]

(* I didn't realize React testing library automatically uses createRoot when it's available. We should ask them (and by "them" I guess I mean @eps1lon) not do that, since there are enough subtle batching changes that it's likely to break some existing tests if they all get automatically switched over when you upgrade React.

We specifically do this because you made it clear in your messaging that createRoot will be the default in React 18. It's also a bad impression to trigger warnings by default (ReactDOM.render is deprecated) in React 18.

I want to stress that warnings are very frustrating for library users in test suites since many suites are configured to fail if any warnings are triggered. So "it's just a warning" isn't helpful advise.

It's a bit odd now to hear that we should not default to createRoot when React itself considers it the default going forward. I'll see if more repositories report integration issues. Though from my impression doing this work in Material-UI we had relatively few places where act() was missing. But that test suite is also maintained by myself so I may have a bit of a bias towards how well act is integrated into existing React test suites. DOM related tests usually only break if they use imperative APIs such as click or focus. All the testing-library APIs are already wrapped in act. I suspect you use a bunch of setState in the tests that weren't wrapped in act before?

But the messaging is still a problem in my opinion. I don't have a strong opinion what should be the default going forward. But the ecosystem should agree on one, not many.

Edit: Next.js also uses createRoot by default if React 18 installed: https://nextjs.org/blog/next-11#upgrade-guide

[markerikson]

Will respond in more detail on the morning, but quick thoughts :

• yeah, I need to bump the renderer package
• we've been assuming that v8 would require React 18 as a min peer dep, just like v7 required 16.8 for hooks. So, I'd want to stick with 18 for testing. That said, if 16/17 compat is viable, then A) we'd need to figure out a good way to run tests against each version, and B) the tests would have to account for some differences in behavior like batching. Like, I just saw earlier that the shim produces an extra render in some cases than the built-in api does.

[markerikson]

I rebased this against master, and squashed a bunch of temp commits together to clean up the history. I also fixed up some types issues. Tests pass, types pass, the CSB build looks like it runs and compiles okay in an example app. I think it's time to merge this PR and publish an initial v8 alpha so that people could start trying it out.

I still haven't tried to implement an actual API for server snapshots. I'll tackle the SSR thing in a follow-up.

I've updated the React peerDep to list 16 || 17 || 18 (although I don't know how to list "experimental" builds here). Per prior comments, I'm generally planning to make this 18-only by the time it's out.

Related to that, I'm seriously considering removing the use-sync-external-store shim dep, and inlining the logic from useSyncExternalStoreExtra back into useSelector. Otherwise, the logic from the useSyncExternalStore shim function ends up in the final app bundle even when you're using the built-in uSES function, and that's just a waste of bytes. If we're not going to support 16/17 via the shim in the final v8 release, then I don't see a point in having that dep at all.

Also, I had removed use of Subscription from useSelector, but that still ends up in the app bundle because Provider has to use it in case there's some connect usage in the app. So, can't get away from that.

[acdlite]

@markerikson We could publish a version of the "extra" module that relies directly on the native API, instead of using the shim indirection. That way if multiple libraries depend on it, users only download the code once.

[markerikson]

@acdlite yup, that'd definitely be another possibility, and I'd be good with that.

[markerikson]

Also FYI to anyone reading, these changes are now live in https://github.com/reduxjs/react-redux/releases/tag/v8.0.0-alpha.0 !

[brainkim]

If we're not going to support 16/17 via the shim in the final v8 release, then I don't see a point in having that dep at all.

My understanding is that the shim will support post-hooks versions once the API is stable.

[markerikson]

My point is that the "shim" part is only useful if we planned to have v8 be compatible with 16/17. But, I was already planning to enforce a hard switch so that React-Redux v8 requires React 18, in the same way that React-Redux v7 required 16.8+ for hooks support.

As currently written, the use-sync-external-store package will always end up including the shim code itself in the final app bundle, even if you're using a version of React that does have a real uSES. If we really do hard-require React 18, then that's a useless 750 bytes for end users that wouldn't even run.

[acdlite]

As of facebook/react#22662, if you import use-sync-external-store/with-selector it will compose the built-in React API, not the shim. (The shim version is available as a separate import: use-sync-external-store/shim/with-selector.)

[Kasdy]

Also, I had removed use of Subscription from useSelector

Hello @markerikson,
shouldn't this bring back the old quadratic algorithm and slow unmount of many components (#1523)?
Since Redux unsubscribe is passed to useSyncExternalStore and it relies on indexOf\splice (like in version <7.2.0)
Thank you

[markerikson]

@Kasdy : not sure, but if there's a problem here it's probably going to be a React issue rather than a React-Redux issue specifically.

Could you try to set up a test case with both React-Redux v7 and React 17, React-Redux v8 + React 18, and also "just" React 18 + a fake little store implementation + useSyncExternalStore, and see what the actual behavior looks like?

edit

FWIW, after a quick glance I don't see any uses of indexOf or splice in React-Redux itself at this point. If you look at the useSyncExternalStore shim implementation there's no list of listeners there - it's just a useEffect directly returning whatever unsubscribe function we give it.

Hmm. However... because we're now using store.subscribe directly, that does use indexOf/splice.

Okay, yeah, this could be an issue.

Could you go ahead and set up those benchmarks anyway, just to confirm that that this is indeed a problem?

One more thought: <Provider> still relies on Subscription anyway, so it's going to end up in the bundle regardless. We probably should update useSelector to rely on that again.

[Kasdy]

@markerikson

Hmm. However... because we're now using store.subscribe directly, that does use indexOf/splice.

Yes! that's what I meant :)

I adapted the benchmark code in #1523 to use Hooks and tested:

React 17 + React-Redux v7.2.6

React 18 + React-Redux v8.0.0

const store = createStore(
    (state = 0, action) => (action.type === "INC" ? state + 1 : state)
);
const increment = () => ({ type: "INC" });

function App() {
    const state = useSelector(s => s);
    const dispatch = useDispatch();

    const [children, setChildren] = useState(0);

    const toggleChildren = () => setChildren(c => c ? 0 : 50000);

    return <div>
        <button onClick={toggleChildren}>Toggle Children</button>
        <button onClick={() => dispatch(increment())}>Increment</button>
        {[...Array(children).keys()].map(i => <Child key={i}/>)}
    </div>;

}

function Child() {
    const v = useSelector(s => s);
    return <div className="child">{v}</div>;
}

[markerikson]

Gotcha.

Could you file a new issue so we can keep track of this separately?

Also: I would really like to have a unit test that captures this intended behavior so that we don't regress again, but off the top of my head I don't know how you'd actually validate this experimentally (especially since perf-sensitive behavior is not a good fit for unit tests that run in C).

Also also: if you have time to file a PR that changes useSelector back to using Subscription again, that would be great!