Remove useEffect from useSyncExternalStoreWithSelector

[ku8ar]

Summary

The motivation for this PR is the following comment:

Early performance benchmarks show parity with React-Redux v7.2.5 for both connect and useSelector, so we do not anticipate any meaningful performance regressions.

https://github.com/reduxjs/react-redux/releases/tag/v8.0.0-alpha.0

Until now, I thought that useMutableSource/useSyncExternalStore would be a much more efficient solution than the current useSelector from react-redux because it would use Fiber's internal algorithm instead of using multiple hooks like useLayoutEffect, useMemo, useState, etc.

But I saw this comment, then read the whole discussion about selector stability (which I thought went the wrong way) and realized that a useSelector based on useSyncExternalStore would have the same performance.

So to optimize the code a bit, I removed the useEffect. Smaller Fiber, less phases.

The code isn't pretty, but it's also not much bigger than before, so it shouldn't make it harder to read.

How did you test this change?

• prettier passed correctly
• eslint passed correctly

To test the changes, I ran yarn run test. There were some errors, but they probably have nothing to do with the provided code:

    Expected: "Cannot read property 'trim' of undefined"
    Received: "Cannot read properties of undefined (reading 'trim')"

[markerikson]

Hey, I wrote those release notes :)

This change looks kinda questionable to me. It's changing the behavior of the non-shim version of useSyncExternalStoreWithSelector so that it's mutating a ref while rendering. I'm also really not sure that removing a single useEffect is going to have any meaningful perf improvement here.

The main reason I even brought up perf in the first place was that React-Redux v6 tried to pass down state updates via context inside of connect, which proved to be a bottleneck:

reduxjs/react-redux#1177

FWIW useSelector itself is unlikely to have any perf issues because it does not have to follow all of the constraints that connect does, and that allows a much simpler implementation. uSES is very similar to useSelector in both concept and implementation, so I wouldn't expect perf issues here either.

read the whole discussion about selector stability (which I thought went the wrong way)

I am curious, what did you mean by this?

[salazarm]

In general writing to a ref and reading from that ref in a single render pass is not safe in concurrent mode because the render might not actually commit and so any side effects that happen as a result of that (eg: if (ref.current) { doSideEffect(); }) would lead to wrong behavior, potentially even an infinite loop if the side effect changes a store that causes the current tree to re-render since when it re-renders React would have reset all refs and state since it determined they won't commit (eg. if something in the tree suspends or the component is offscreen).

Also the failure might be because this line would be wrong after this change.

[ku8ar]

@salazarm I now this is "hacky", but in this case we are dealing with "memoizedSelector" function, which is a pure function that returns a value, and the mutable "inst" object only exists to hold the current value and compare it to the next call. So this changed logic should not cause problems in concurrent mode. And I can't imagine a case where the selector is impure...

@markerikson I was using old versions of react-redux based on context, understand you.

As for the stability of selectors, the initial API (invented by @acdlite I think) was a better idea for me.
Of course, I perfectly understand your noble reasoning in which you insisted on keeping "performance backwards compatibility". But from my point of view, if a new version of library will be released, there would be no problem to inform users of this library that they should create stable selectors. In most cases, developers were using libraries like reselect, so those selectors were stable anyway. And even if such users updated the library without fixing the code, the code would still work, it would just be less efficient (according to the initial useMutableSource assumptions).
So it's a bit of a conflict on the line: better is the enemy of good. I also personally don't like arrow functions in selectors because they are ugly.

[markerikson]

@ku8ar I feel like you're kinda mixing up a few different things here.

First, if all users of React-Redux and Zustand suddenly had to make their selectors stable, this would be a huge breaking change for their codebases. Every usage like const counter = useSelector(state => state.counter.value) would have to be wrapped in useCallback first instead. Part of the goal for both React 18 and React-Redux v8 is to let users upgrade with minimal changes. Even if it's a major version, we want to minimize disruption to all of our users.

After we discussed that issue with Andrew, he determined that uSES could be implemented without needing to restrict behavior that way, which is a win for everyone.

Second, arrow functions have nothing to do with selectors in and of themselves. You can write selectors with the function keyword if you want to - it's just going to take a bit more code. That's a personal preference, and I don't see the connection here.

[ku8ar]

@markerikson You didn't quite understand me.

This is a hook with selector currently suggested by your doc:

const counter = useSelector(state => state.counter.value)

And this is a hook with a selector without a dynamically generated arrow function (stable, which I wrote about earlier).
We don't create unnecessary functions. Each function is named (nice convention). Named functions also can be separated into different files (as part of the business logic). Just clean code.

const selectCounterValue = state => state.counter.value
// ...
const counter = useSelector(selectCounterValue)

Using this convention, most selectors are stable. Of course, for many ugly projects, this would require a refactoring. But by cutting away from unstable selectors, react-redux would become something of a framework whose requirements suggest writing beatufiul code (stable, pure and named functions).

But unfortunately backwards compatibility won.

OK, I guess there's no point in arguing further. Happy holidays ;)

[markerikson]

But unfortunately backwards compatibility won.

Yes, because we don't want to break user code without absolutely good reason. That's part of the responsibility of maintaining a widely used project. "I think this code looks better if you extract the functions" is not a good reason.