Fix subscriber being overwritten

[JeremyRH]

@jhgg
Should fix #84
Here was the problem:

• Subscriber object is created during the render phase and its index is assigned.
• setState is called before the subscriber object is added to subscribers array (between render phase and useLayoutEffect).
• subscribers array is made dense, changing the length of the array and indices of subscribers.
• Subscriber object is added to subscribers array at wrong index, overwriting a valid subscriber.

I moved the code that makes subscribers a dense array into the "unmount" phase (return function of useLayoutEffect callback).

[drcmda]

Thanks, interesting solution using Array.filter! 🙂

[drcmda]

made a patch release

[paulshen]

I think there might still be an issue with concurrent mode. A component can unmount between another component's render and useEffect. Here is an example. https://codesandbox.io/s/fervent-heisenberg-vtqiw (forked from React docs). Click the button, look at the console, and notice that HomePage unmount happens between ProfilePage render and ProfilePage effect. With respect to zustand, I think this can lead to the same clobbering behavior of compressing the array when there is a pending index.

My hunch is that we need to avoid compressing the array when there is a pending index, although tracking this is tricky with React Suspense.

[JeremyRH]

@paulshen I think you're right. The problem with waiting on a pending index is a component can be called during the render phase but interrupted before React renders it in the DOM. We'd have an always-pending index and never be able to compress the subscribers array. I want to get away from doing anything during the render phase and only use useEffect for adding/removing subscribers.

Unfortunately, useEffect callbacks are called from child to parent. This means the subscribers array would not be ordered properly when we loop over it. We can loop over the array in reverse or add subscribers to the beginning of the array but that doesn't work after subscribers are added a second time:

Initial subscribers: [4th, 3rd, 2nd, 1st]
After adding new subscribers: [4th, 3rd, 2nd, 1st, 7th, 6th, 5th]

We could have two arrays for subscribers, one that is sorted and one for holding new ones:

sorted: [1st, 2nd, 3rd, 4th]
new: [7th, 6th, 5th]

Then we reverse iterate over the new array and add the values to the sorted one. I don't know how to detect when to do this. It has to be done after React flushes effects. Maybe a combination of useLayoutEffect and useEffect.

[paulshen]

I don't know if there is a solution here (yet) 😞It feels like the root fix is having a useEffect variant that fires in parent -> child order (kinda like DOM event bubbling/capturing) but I'm guessing that's not on React's roadmap.

Redux has an interesting solve for its <Connect> component API using context to maintain parent-child relationships (see Subscription) but it does not have a solve for its hooks API since hooks cannot render a context provider.

[JeremyRH]

I don't expect a useEffect variant like that to ever exist but it would solve our issue.

It seems like the first useEffect callback is called after all useLayoutEffects callbacks are called. At least, this is the behavior for react-dom. I wonder if useLayoutEffect can be used to add subscribers to a newSubscribers array and then useEffect would check that array length and pop the items into a sortedSubscribers array.

When we want to call all subscribers, we would just call all sortedSubscribers in order then call newSubscribers in reverse.

The problem with this solution is it's dependent on the reconciler's behavior.