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 3cc4a80:

AHHHH that makes sense!

I guess the fix is not fully correct since tests fail 🤣 but nevertheless I think that it would fix the mentioned issue. It would be interesting to understand how the subscription nesting works and how it depends on this isMounted stuff to figure out a solution that would cover both requirements.

So basically with this change, we have 2 affected tests:

• should not notify nested components after they are unmounted
• should enforce top-down updates to ensure a deleted child's mapState doesn't throw errors

The first one fails because mapStateToProps gets called more times - it's not ideal but potentially wouldn't necessarily have to be treated as a big problem. However, the second one is more concerning and after some light analysis, I think that actually both failures are related to the same thing. I could be mistaken though.

So what's the problem? The problem here is that mapStateToProps for a given component is called before that component has a chance to get unsubscribed. This again is related to the usage of a layout effect to manage isMounted. With that, the component was treated as unmounted "sooner" and the notification from the parent (originating in its layout effect where captureWrapperProps leads to notifyNestedSubs) was simply being skipped. The useSyncExternalStore is a passive effect though so it gets unsubscribed later and thus we no longer skip this notification.

Interestingly, I went back in time to check out something and in 7.x this isMounted wasn't in the code at all. That's because the subscription to the store was not passive. It was also managed from a layout effect:

I've noticed one additional thing in the current implementation:

Perhaps if this could be refactored to get more "inline" with the intended usage the problem would go away. I assume that in such a case React's implementation wouldn't rerender this component at all as it was already unmounted in its internal data structures and mapStateToProps wouldn't get called at all. This would change the timing of mapStateToProps calls though and perhaps it would have some performance implications as React would get notified about potential changes more often. However, I would also assume that it could mostly bailout from unwanted rerenders thanks to the /with-selector. You would know better though as you probably have already tried the less hacky usage of that hook and it didn't play out well for you 😅

Yeah, the reason for all that "non-standard" usage of uSES is that connect cares about more than just "the store state" or "the result of mapState" having changed. It has to combine together the results of mapState + mapDispatch, and optionally use the results of mergeProps if that's been supplied, and then use whatever that combined object is to decide if the child component should re-render.

I couldn't come up with a good way to break apart that logic, so I ended up with an implementation where connect is still doing the bulk of the comparison work.

After further investigation, I've concluded this is actually a bug in the 03-25 experimental React build Replay is using - it works as expected in a current (08-17) React experimental build.

See #1940 for discussion.

Thanks for the attempted fix!