Stale props issue in selector API

[dai-shi]

Thanks so much to reduxjs/react-redux#1179 (comment), it explains better to understand.

So, as long as we use useReduxState(), we are safe, but with selectors there could be inconsistencies. In our case, it's useReduxSelectors().

[dai-shi]

it('ensures consistency of state and props in selector', () => {
  let selectorSawInconsistencies = false

  const Parent = () => {
    const count = useReduxState()
    return <Child parentCount={count + 1} />
  }

  const Child = ({ parentCount }) => {
    const result = useReduxState(count => {
      selectorSawInconsistencies = selectorSawInconsistencies || (count !== parentCount)
      return count + parentCount;
    })

    return <div>{result}</div>
  }

Are you sure we need + 1 in return <Child parentCount={count + 1} />? @MrWolfZ

[MrWolfZ]

@dai-shi yeah, that was from an old version of that test. Just pass count through. I have edited my post in the hooks API design issue.

[dai-shi]

Thanks!

[dai-shi]

For the first test, I forgot to add try-catch, even though I saw the code in the other issue.

For the second one, I'm not sure if this is really a blocking issue. If we allowed a selector to access global variables, there could be other issues not related to stale props. (and I remember I saw many impure selector code out there...) But anyway, I understand react-redux needs to care a lot about backward compatibility.

[dai-shi]

@MrWolfZ
If we want top-down subscriptions to resolve the stale props issue only with hooks (no hoc), here's my crazy idea. Hooks don't know about renders so we can't build nested subscriptions, but we might be able to guess which subscription should called prior to other subscriptions. I'm not 100% sure if this heuristic approach really works nor if React renders as expected in between subscription callbacks (but that's what we experienced without batchedUpdates.) Anyway, we probably never take this approach because of bad performance, but if this is a new idea, someone like you may find better alternatives.

the reference commit: ff9957b

[MrWolfZ]

@dai-shi In general I think the heuristic to simply force components that get rendered first to be updated first should work out, except in concurrent mode. Concurrent mode means two major issues I can see:

1. A setState call or similar on a store subscription callback doesn't mean the component and its children get updated synchronously, so you can still get stale props (react-redux works around this by only notifying children after the parent was committed to the DOM inside an effect)
2. The impure render method may cause a memory leak since a render may run (thereby adding the fingerprint to the store) and then the result gets thrown away, which means the component is never mounted and unmounted, so the fingerprint is never removed

Not being able to use batching could also lead to performance issues.

[dai-shi]

Thanks. Yeah, I'm pretty sure it doesn't work in concurrent mode.

only notifying children after the parent was committed

With the heuristical list, we can do the same like this, can't we?
(We can't solve the performance issue anyway, though.)

a memory leak

Oh, you are right. There can be a memory leak even if we useRef.
It might be better to use a WeakMap instead of an array of symbols, but it's still tricky.

[MrWolfZ]

With the heuristical list, we can do the same like this, can't we?

No, I don't think we can properly do this since we don't know the tree shape and therefore would have issues updating the component tree optimally.

Let's quickly discuss why notifying children is even required. When a connected component is notified of a change, this usually leads to its children being re-rendered (in which case notifying children is not even required), but in the case there is for example a non-connected memoized component in the component's subtree, this could prevent children from properly re-rendering. So, in most cases this leads to a single DOM update that processes the store update completely, but to handle the edge cases react-redux notifies the children after the DOM changes are committed.

Let's illustrate the usual case with an example component tree:

<Root>
  <A>
    <AA />
  </A>
  <B>
    <BB />
  </B>
</Root>

In this case, let's say A and B are interested in an update. Both get notified of the update at the same time, and they re-render their children. Once this is committed, AA, and BB are notified, but since they were already re-rendered they ignore the store update.

Now let's see how this would work with a linear list of fingerprints. Since we don't know whether fingerprints later in the array are children or siblings we simply have to defer updating all fingerprints later in the list until after the current component is committed. This means for the example above that first, A is rendered and committed, before then B is finally rendered. Therefore, if you have many independent connected components in the tree you will get cascading updates that span over multiple renders, which is bad for performance and could even lead to visual artifacts.

[dai-shi]

if you have many independent connected components in the tree

This totally explains. Thanks a lot.

---

While you are here, do you think react-redux v7 accepts this stale props issue as a limitation of hooks api? I think your proposal is the best so far and it's silly to introduce hoc to solve the stale props issue. It would have been nice if the state context (aka v6) were possible, but it not likely to happen in the foreseeable future.

[dai-shi]

I guess the discussion in reduxjs/react-redux#1252 explains it.

---

So, in conclusion, our useReduxState doesn't have stale props issue because it doesn't remember anything, and useReduxSelectors has the issue as well as the official react-redux hooks (and probably all other unofficial hooks) which we'd basically live with as a limitation.

@faceyspacey You asked somewhere for the clarification, here it is.