Connect HOC misses store changes when "offscreen"

[bvaughn]

What version of React, ReactDOM/React Native, Redux, and React Redux are you using?

• React: 0.0.0-experimental-e7d0053e6-20220325
• ReactDOM: 0.0.0-experimental-e7d0053e6-20220325
• Redux: 4.2.0
• React Redux: 8.0.2

Here is a Replay recording of this bug with some annotations from me:
http://app.replay.io/recording/replay-failing-reactdevtools-e2e-repro--3351989d-95d4-4035-80af-07d813a623aa

Here is a Loom (it's long) where I use Replay to investigate the problem:
https://www.loom.com/share/3d60c14466184b03acc9af7b80e9eff8

Also note that this issue is specific to an experimental React API, Offscreen. The React team has published some early documentation about this API but it has not been released in a stable form yet.

What is the current behavior?

A connected component misses updates that are fired while it is inside of a hidden Offscreen component.

I believe this is because the component unsubscribes when it is unmounting and does not check for missed updates when it is later remounted. (The selectors passed tot he map-state-to-props function were not executed.)

What is the expected behavior?

The expected behavior is perhaps up for debate. Should the component process Store updates (at a lower priority) while hidden or should it wait to render them only if it's later re-shown? In a sense, this question isn't worth debating yet because I don't think React currently provides an API (even an experimental one) to stay subscribed while in a hidden state.

That being said, I think the currently expected behavior is that when then Offscreen tree is later re-shown, React Redux connect should check the Store for the latest values and re-render if they have changed since the component was last rendered.

Which browser and OS are affected by this issue?

Presumably all of them

Did this work in previous versions of React Redux?

• Yes

[Andarist]

In a sense, this question isn't worth debating yet because I don't think React currently provides an API (even an experimental one) to stay subscribed while in a hidden state.

Do you think such an API is a good idea? I got an impression that the whole strict effects stuff was introduced specifically to avoid subscriptions in hidden components etc.

---

As to the shared Replay - I couldn't resist taking a look at it 😅 I'm not sure if I understand the issue correctly in full so take my findings with the grain of salt.

At the moment, connect is based on useSyncExternalStore, see here. So I've went to the subscribeForReact function that gets passed to the useSyncExternalStore and added a log there. I've also logged WrappedComponent.name there to get a better insight about the surrounding context for a call.

Note that you click on that React tab somewhere around 0:27 of this recording and this subscribe is never called by React after that:

The very last call to this subscribeForReact is coming from the ObjectInspector but it happens around 0:25 and that is still before you click on the tab.

We can see here your click around 0:27, this last call to subscribeForReact at 0:25, and even the last render of the ReactDevtoolsPanel that finally rerenders correctly at 0:32 (second line from the bottom, after triggering some additional update?):

So it looks as if React just reused the hidden content, without rendering it/resubscribing to it. I assume that the lack of rerender is good when we know that nothing has changed inside but it shouldn't skip resubscribing it because it never gives a chance for this component to "fix" its state.

Since you have fixed it by migrating to the hooks API (which is also based on useSyncExternalStore), there has to be more to it to end up with this bug though.

[markerikson]

@Andarist have I mentioned lately that you're awesome? :)

Yeah, my initial thoughts here are:

• I did some stupid hacky things to get connect working with useSyncExternalStore. So, it doesn't at all surprise me that there's edge cases where something breaks.
• connect also does a ton of memoization inside
• But yeah, I would also have assumed that at some point on remount it would have tried to resubscribe

[Andarist]

@Andarist have I mentioned lately that you're awesome? :)

It has been a couple of days since the last time 🤣

I did some stupid hacky things to get connect working with useSyncExternalStore. So, it doesn't at all surprise me that there's edge cases where something breaks.

Yeah, I wouldn't be totally surprised by that :P

But yeah, I would also have assumed that at some point on remount it would have tried to resubscribe

But because of this, I'm inclined to believe that this is a React issue here. Whatever memoization/hacks you have in place shouldn't matter here because they could only mess up an active subscription from React but it seems like React has unsubscribed from this store and never subscribed back. So it didn't even give you a chance to screw up :P

[bvaughn]

But because of this, I'm inclined to believe that this is a React issue here. Whatever memoization/hacks you have in place shouldn't matter here because they could only mess up an active subscription from React but it seems like React has unsubscribed from this store and never subscribed back. So it didn't even give you a chance to screw up :P

Maybe I'm misunderstanding something, but why would React not unsubscribe from the store in this scenario? There is (at least currently) no proposal that I'm aware of to stay subscribed while offscreen/hidden.

The thing that surprises me is that the connected component doesn't re-render when being shown again (so no chance to check for a missed subscription). Then again, the component with the new hooks did. That seems like an interesting difference.

Did the connected component not re-render because of a React screw-up or a bug in Redux memoization? Did the component with hooks re-render because of missing memoization or because it was working as designed? 🤷🏼

[phryneas]

Did the connected component not re-render because of a React screw-up or a bug in Redux memoization? Did the component with hooks re-render because of missing memoization or because it was working as designed? 🤷🏼

My guess: It's being called with the same props as last time and it's wrapped in React.memo? Still, that shouldn't happen - React should run another render on that even then. Maybe something to take up with the React team if my assumptions proves valid?

[Andarist]

Maybe I'm misunderstanding something, but why would React not unsubscribe from the store in this scenario? There is (at least currently) no proposal that I'm aware of to stay subscribed while offscreen/hidden.

I meant that this hidden component was unsubscribed (although I didn't fully confirm that, but it's what should happen) but I never have seen it being resubscribed on this Replay. That indicates to me that it's possibly a bug in React. But, of course, I could also draw incorrect conclusions from the recording altogether.

Still, that shouldn't happen - React should run another render on that even then. Maybe something to take up with the React team if my assumptions proves valid?

To be more precise (if my findings are correct) - it wouldn't really have to rerender this component unconditionally. In this situation, it should resubscribe external stores, and a change in the snapshot/selected value should lead to a rerender. OTOH, we can't subscribe immediately when showing the component as this is a side-effect, and thus React needs to defer that to a commit phase. So the only way to check if there is a change is to actually run the getSnapshot when rerendering a component, so this could "force" the rerender of the component for this purpose. I think there is a potential to avoid rerendering the whole component - since React already has a reference to the getSnapshot, it could call it on its own if there is no other reason to rerender and only rerender if a change is detected. However, that would just be a potential optimization for the revealing of the hidden content. Regardless of this all effects should be setup again - so perhaps rerendering the whole component would just be the easiest solution.

I've located the usage of the OffScreen in the Replay's codebase, I'm gonna replicate this issue on the codesandbox later to see if we can distill it into its barebones.

[bvaughn]

We're getting kind of deep into the weeds of speculation here. Bugs in React are always a possibility, but at the same time– I am pretty confident that the team would not have missed something so obvious as "sync external store will miss subscriptions that fire during Offscreen" so I think there must be something else going on here.

FWIW I spent a bit of time trying to recreate this in a Code Sandbox a few days ago but was not successful. Perhaps I overlooked something silly, or perhaps it's more complicated to recreate:
https://codesandbox.io/s/silly-forest-nw7nj2

One interesting thing the Code Sandbox I created above shows is that React actually is still updating the offscreen components when they're hidden. (You can inspect the DOM and verify that their content is being updated).

Recording a profile confirms this (note the high-pri render and the separate Offscreen priority render):

So perhaps a key difference here might be if @markerikson's component is actually not supposed to unsubscribe in the offscreen state?

[markerikson]

I'll try looking at the actual replays tomorrow, but off the top of my head, I would expect this to unsubscribe.

React-Redux v8 defaults to using the useSyncExternalStore "shim" package, which Andrew wrote to be a fully compatible implementation of uSES in userland for user with React 16/17.

That implementation relies on useEffect/useLayoutEffect, and specifically returns unsubscribe as the hook cleanup function:

https://github.com/facebook/react/blob/796d31809b3683083d3b62ccbab4f00dec8ffb1f/packages/use-sync-external-store/src/useSyncExternalStoreShimClient.js#L123-L124

Assuming that React runs hook cleanups when a component goes offscreen... then yes, I would most definitely expect it to unsubscribe at that point.

But in turn, I would also expect that React would re-run the effect hooks when the component is brought back in, and that specifically checks for initial data.

So, those are my expectations. Clearly something about those is not valid :)

[Andarist]

I didn't yet have time to fully validate this but I think I got the fix for this: #1941

What @bvaughn has mentioned 2 comments above got me to the conclusion that this is the most likely culprit. It seems that at the moment OffScreen components don't clean up passive effects - I was 100% sure they were. Note that this was already changed 4 days ago in React, see the PR here. We can even read in the description of this PR:

This changes the behavior of Offscreen so that passive effects are unmounted when the tree is hidden, and re-mounted when the tree is revealed again. This is already how layout effects worked.

So how this has affected React Redux? isMounted ref was set and cleaned up in a isomorphic layout effect (so basically in a layout effect on the client):

react-redux/src/components/connect.tsx

Line 630 in 720f0ba

useIsomorphicLayoutEffect(() => {

But at the same time the passive subscription from the useSyncExternalStore was not cleaned up - and, of course, it was not resubscribed later on since, well, it never got cleaned up in the first place.

So in an effect of all of this - connect was skipping the updates that happened within a hidden tree. And when it was revealed there was no signal for it to recompute stuff. It probably was capable of receiving some future updates though, after the layout effect set isMounted back to true when the component was being revealed.

[markerikson]

AHHHH, that's fascinating!

So apparently this was my mistaken assumption:

Assuming that React runs hook cleanups when a component goes offscreen

or at least for passive effects.

Out of curiosity, what happens if we were to alter the build so that it aliases react-redux to react-redux/next and use the built-in uSES instead of the shim?

[Andarist]

Out of curiosity, what happens if we were to alter the build so that it aliases react-redux to react-redux/next and use the built-in uSES instead of the shim?

What scenario are you thinking about here? Btw. when using React 18 the shim should proxy to the "native" implementation - so I would say that "the result is always the same".

[markerikson]

Oh yeah, that's a good point - that explains why I was seeing 0 hits for the actual shim logic, and 1 hit for the outer part of the shim file :) (forgot that was the case - I was fixated on the fact that we do always load the shim library, which does add to bundle size)

[bvaughn]

What @bvaughn has mentioned 2 comments above got me to the conclusion that this is the most likely culprit. It seems that at the moment OffScreen components don't clean up passive effects - I was 100% sure they were. Note that this was already changed 4 days ago in React, see facebook/react#24977. We can even read in the description of this PR:

Just to be clear, my Code Sandbox is running react-dom 18.3.0-experimental-32baab38f-20220811 (built from commit facebook/react@32baab38f, which was after facebook/react@80f3d88, so it should be showing the latest Offscreen/uES behavior).

However the version of React that Replay is currently building with is npm:0.0.0-experimental-e7d0053e6-20220325 (built from commit facebook/react@e7d0053e6, which is way older)