Zustand 2.x // Ensure order of subscribers

[JeremyRH]

This could be a fix for #64.

The original issue was due to the current behavior of useEffect. It seems effects in child components are called before parent effects. We subscribe to the store in useEffect so child listeners were added to the array before parent listeners. I changed it so the enumeration order should always match the render order. Now child listeners will be removed before they can be called if the render is synchronous.

One thing to note, we're adding a key to a mutable object during React's render phase.
Edit: I've removed the need for mutating an object.

[drcmda]

amazing that you tried, i thought it is impossible to track render order with hooks? at least that's what i've read. when components mount and unmount conditionally, like inside a list of components it goes away and comes back again in the middle, won't it shift?

[JeremyRH]

Hooks need to be called unconditionally so everything in the function body of a custom hook runs during the render phase.

If the component unmounts, the useEffect's cleanup function will be called, removing the listener. If suspense interrupts the render phase, the listener is not added. Only a null value is added as a placeholder during the render phase.

[drcmda]

Incredible. One more dumb question, say it fires in order, parents first, one parent decides to not render a child (which uses useStore and would be next in line in zustands listener array ), when zustand has come back from the parents immediate render pass, has react already unmounted the child and therefore killed the listener?

[JeremyRH]

Yeah, that seems to be the case with a synchronous render.
I want to test this more to see exactly what happens with an asynchronous render.

[drcmda]

Is this enough to switch it to async?

ReactDOM.unstable_createRoot(document.getElementById('root')).render(<App />)

haven't tried much with it yet.

When it's async, then listeners won't cause render passes, similar to how batchedUpdates work. Triggering selectors can potentially fail still, but they're wrapped inside a try/catch for that reason. Once the frame is over React should have gathered all dispatches and now execute a single top-down render pass. Selectors are allowed to throw exceptions in the render function, but leaf components shouldn't render before their parents so all should be good.

Btw, do you see any obvious perf regressions or regressions in general with your solution?

[JeremyRH]

We should test batched updates and render interrupts to see if there are issues. I'm not familiar with unstable_createRoot either but it seems straightforward to use.

As for potential performance issues, there's a chance for null values to be accumulated on the listeners object and never cleaned up. You would have to call useStore in your component and afterwards interrupt the render with suspense or something. Maybe we can find some way to clean up the null values.

[markerikson]

Not familiar with the code here, but I would be very skeptical that any attempt to track ordering for multiple nested subscribers as a single list will work correctly. I would also recommend against tracking things based on mutations during the render phase - even if it appears to work now, that will definitely be Concurrent Mode unsafe. There's some very related comments from one of the React-Redux hooks discussion threads at reduxjs/react-redux#1179 (comment) and reduxjs/react-redux#1179 (comment). Basically, even if an initial render pass lists things bottom-up correctly, that doesn't account for future components being added and removed over time, and there's no way to track how those fit into the component tree

React does guarantee that when a render is committed, the various lifecycle methods and effect callbacks (componentDidMount/Update, useEffect/useLayoutEffect) will fire bottom-up for the components that rendered this pass.

For React-Redux, our solution for top-down updates is to put our own custom Subscriber class into context, and have connect wrapper components subscribe to the nearest connected ancestor and then override that context value with their own Subscriber instance for descendants to listen to. This unfortunately doesn't work with hooks, because hooks can't render context providers.

A couple other observations on this PR specifically:

• Why are you using a Record to "maintain iteration order"? Yes, JS objects now have a fairly consistent iteration order. But, I'm not seeing any indication of what listenerIndex values look like when they're passed in, and the default values seem to be Symbols instead. If you're concerned about ordering, why not stick with an array?
• If the point is to iterate over subscribers top-down, shouldn't this be looping over the entries in reverse order or something, giving that they'd be added bottom-up?
• Related, I don't see where listenerIndex is being passed in at all.

[dai-shi]

We had some discussions about heuristic approach. may or may not be relevant.

It this somewhat related? reduxjs/react-redux#1276

[JeremyRH]

@markerikson

I would be very skeptical that any attempt to track ordering for multiple nested subscribers as a single list will work correctly.

You might be right but what exactly are we trying to do here? In our case we want to call a listener set by the parent before calling listeners set by its children. We don't necessarily care about the order, the only thing that matters is parent listeners are called before their children's. Here's an example:

Parent: 1
  Child: 2
    Child: 3
Parent: 4
  Child: 5

Let's say Child 2 changes and becomes a new component. Here's how it would look:

Parent: 1
  Child: 6
    Child: 7
Parent: 4
  Child: 5

It doesn't matter that the listeners would be called in this order: 1, 4, 5, 6, 7. That still works because every parent listener is called before its children's. This would break if children are "swapped" from one hierarchy to another or parents are initially rendered after their children. I don't know if that's possible though.

I would also recommend against tracking things based on mutations during the render phase - even if it appears to work now, that will definitely be Concurrent Mode unsafe.

My understanding is that it's unsafe because the render could be interrupted and you're left with mutated state but no render. I tried to do a "safe" mutation. Something that won't be a problem if the render is interrupted. For example:

const listeners = {};
function Component() {
  const listenerKey = useRef(Symbol());
  if (!listeners.hasOwnProperty(listenerKey.current)) {
    // Set a null placeholder to keep object iteration order matching render order.
    listeners[listenerKey.current] = null;
  }
  ...
  useEffect(() => {
    // Set the value to the actual listener once React has committed the initial render.
    // Reassigning a value does not change iteration order.
    listeners[listenerKey.current] = listener;
    return () => delete listeners[listenerKey.current];
  }, []);
}

When the listeners object is iterated over, we have to check for null but this does seem to be a "safe" mutation if we can figure out how to clean up stale null values.

Basically, even if an initial render pass lists things bottom-up correctly, that doesn't account for future components being added and removed over time, and there's no way to track how those fit into the component tree

Thanks for the links! There is so much great info in these.
About your comment; I don't think we need to track how those fit into the component tree, we just need to ensure parent listeners are called before their children's.

Why are you using a Record to "maintain iteration order"? Yes, JS objects now have a fairly consistent iteration order. But, I'm not seeing any indication of what listenerIndex values look like when they're passed in, and the default values seem to be Symbols instead. If you're concerned about ordering, why not stick with an array?

Sorry, the code is not very readable. I'm hoping I can clean it up.
Using an array would mean keeping an incrementing counter to be used as the index. I use symbols instead. The default value is because subscribe can be called externally by our users.

If the point is to iterate over subscribers top-down, shouldn't this be looping over the entries in reverse order or something, giving that they'd be added bottom-up?

That would be a nice way to solve it, depend on the bottom-up order of useEffect. After a render is committed, the batch of useEffects are called and the listeners are added (in reverse). What would happen to the next batch of listeners after another render is committed? I don't know how you would figure out how to place them.

Related, I don't see where listenerIndex is being passed in at all.

Sorry, I know it's not very clear, here it is:
https://github.com/react-spring/zustand/pull/65/files#diff-f41e9d04a45c83f3b6f6e630f10117feR122

[dai-shi]

@JeremyRH Just read the code. I'm not very familiar with the internal of zustand, but it looks pretty similar to my trial: dai-shi/reactive-react-redux@ff9957b

Apart from Concurrent Mode, my biggest concern back then was that I wasn't able to use batchedUpdates with this approach, which was critical for performance.

[JeremyRH]

@dai-shi Ah, I think I understand now. Using batched updates would break things because regardless of order, child listeners would not be removed synchronously after the parent listener is called. We currently don't use batched updates in Zustand but we definitely should.

[drcmda]

Batched updates would remove cross platform capability though, or at the very least tie it to react. I think it would make sense though to add a Middleware for this, and later, hopefully soon, react will be async by default.

Jeremy, what's your impression after reading through marks statement, are we good? Can we merge it?

[JeremyRH]

@drcmda I think this is reasonably safe to merge but this is technically a breaking change. Because it's a breaking change, I decided to work on some other issues too. Added support for #41 and #50 and fixed #46 and #47. I also removed the "the 2nd arg for dependencies was deprecated in 1.0." warning and argument fixing because I feel it's okay to remove deprecation warnings in a major release. Let me know what you think.

[JeremyRH]

Here are the breaking changes:

• Removed shallow.cjs.js, shallow.iife.js, middleware.cjs.js, and middleware.iife.js. They are now only using the CommonJS format and are named shallow.js and middleware.js.
• StateListener type has changed. An overloaded signature was added for better error handling.
• Subscribe type has changed. It uses the updated StateListener type.
• api.subscribe has a new function signature that removes the object wrapper.
• listeners are iterated in a different order than they were before. Parent to child rather than child to parent.
• The fix for the second arg of useStore was removed. useStore won't stop the Uncaught TypeError that will be thrown if you pass in an array as the second arg.

[drcmda]

wow, that's awesome. let's merge. 🎉

one last question, with the new typing, do you you see an opening for an easier way to handle the subscribe function? this is the one thing that bugs me about the api b/c i can never remember it, and i use subscribe tons with threejs and pass-through state reactions.

hooks:

const all = useStore()
const x = useStore(state => state.x)
const { x, y } = useStore(state => ({ x: state.x, y: state.y }), shallow)

current subscribe:

api.subscribe(x => console.log(x, "everything's changed"))
api.subscribe(x => console.log(x, "has changed"), { selector: state => state.x })
api.subscribe(({ x, y }) => console.log(x, y, "have changed"), { selector: state => ({ x: state.x, y: state.y }), equalityFn: shallow })

without the object wrap ...

api.subscribe(x => console.log(x, "has changed"), state => state.x)
api.subscribe(({ x, y }) => console.log(x, y, "have changed"), state => ({ x: state.x, y: state.y }), shallow)

can't remember exactly what it was but i think it wasn't doable back then. is it still the case?

[drcmda]

btw, about the batchedUpdates middleware, do we want to include one?

const batched = config => (
  set,
  get,
  api,
) => config(args => ReactDOM.unstable_batchedUpdates(() => set(args)), get, api)

or is it not worth it, given that it's platform dependent?

[JeremyRH]

The object wrapper for the subscribe function bugged me too and I remember trying to get rid of it. The problem is we need to update selector, equalityFn, currentSlice, and subscribeError in the useStore hook without resubscribing. We also need to update currentSlice and subscribeError in the subscribe function and get those values back to the component calling useStore. Mutating an object was the easiest way to do this. In fact, I'm not sure how you could update values in subscribe and reference those updated values in a component calling useStore without mutation. Anyway, I created a wrapper function so that api.subscribe now has this signature:

export type ApiSubscribe<T extends State> = <U>(
  listener: StateListener<U>,
  selector?: StateSelector<T, U>,
  equalityFn?: EqualityChecker<U>
) => () => void

About batchedUpdates, I don't think it's worth providing our own batchedUpdates middleware unless we get reports of performance issues. Wouldn't we have to make ReactDOM a peer dependency? It's also relatively simple for someone to write their own.

[drcmda]

awesome!!! 😀 and i agree, injecting batchedUpdates is easy enough to do it in userland if it's needed. so whenever you want to pull the trigger.

[JeremyRH]

@drcmda I think the readme needs to be updated around the api.subscribe function. Also did you want to publish the release? I know you usually advertise new versions on your twitter.

Anyway here are the breaking changes again:

• Removed ESM and IIFE versions of middleware.js and shallow.js. Any imports of the ".cjs" files should be changed from "zustand/{name}.cjs" to just "zustand/{name}" as they are all CommonJS now (index.js still has different formats).
• api.subscribe has a new function signature that removes the object wrapper: api.subscribe(listener, selector?, equalityFn?).
• StateListener type has changed. An overloaded signature was added for better error handling.
• Subscribe type has changed. It uses the updated StateListener type.
• listeners are iterated in a different order than they were before. Parent to child rather than child to parent. It would be rare if this causes any issues.
• The fix for the second arg of useStore was removed. useStore won't stop the Uncaught TypeError that will be thrown if you pass in an array as the second arg.

[markerikson]

Out of curiosity, think you could do a writeup on what exactly actually changed here to resolve the ordering issue?

[JeremyRH]

@markerikson I'll outline the problem that was solved:

Let's say you have a component that renders a list of items and the state is structured in this way:

{
  "1": { text: "child 1" },
  "2": { text: "child 2" }
}

The parent uses a selector hook to get the state and renders the children by looping over their IDs:

function Parent() {
  const childStates = useSelector(s => s);
  return (
    <div>
      {Object.keys(childStates).map(id => (
        <Child id={id} />
      ))}
    </div>
  );
}

The child component uses the same selector hook to access its state:

function Child({ id }) {
  const text = useSelector(s => s[id].text);
  return <div>{text}</div>;
}

Yes, this is a strange way for the child component to access its state, but it should not cause errors.

Zustand and React-Redux add a subscriber to the store when a component uses a selector. These subscribers are added using React's useEffect hook. useEffect callbacks are called first from the children up to the parents per branch:

useEffect: 3
  useEffect: 2
    useEffect: 1
useEffect: 5
  useEffect: 4
useEffect: 6

This means when the subscribers are called, children will be scheduled to re-render before their parents. React-Redux solved this by using a custom Subscription class that keeps a reference to its parent just like a linked-list. The subscriber is not attached until trySubscribe is called in the useEffect hook.

We went a different route with Zustand. There is a subscriberCount variable that we assign and update during the initial render phase of a component. This is used as the index when we store the subscriber in the subscribers array. Subscribers are deleted when they unmount and the holes in the subscribers array are "filled" by consolidating the remaining subscribers into a new array.

I don't remember why I thought this was still a problem in React-Redux. Maybe there is an edge case I'm missing.

[markerikson]

That pattern isn't strange at all, the "parent passes ID as prop, child extracts data based on ID" pattern is exactly how we recommend doing things for best performance with React-Redux.

And yes, this issue is absolutely a problem for us. The Subscription class is a solution, but it also only fully works for connect. This is why our hooks API reference warns about the "zombie child" issue:

https://react-redux.js.org/api/hooks#stale-props-and-zombie-children

One thing that I think may be different is that React-Redux attempts to guarantee that components only read from the store when they've received the latest props from their parent. This requires top-down updates, cascaded. If all subscribers are notified in the same pass, even if it's in the right order, the parents wouldn't yet have had a chance to re-render and pass new props to the children, so the children would be using "stale props" to extract data from the store in their selectors.

Does Zustand deal with that aspect at all?

Also: what happens in Zustand if a new subscriber is added later, but it's actually somehow higher up in the tree than some other subscriber?

[JeremyRH]

Stale prop errors are caught in a try-catch and a flag is set to re-run the selector during the render pass. I'm pretty sure React-Redux works the same way.

Also, I'm not sure how a component higher in the tree can be added later and contain a child that was rendered before it.

[markerikson]

Stale prop errors get caught, yeah, but React-Redux attempts to guarantee that connect is never called with stale props, regardless of whether the mismatched values would result in an error from the mapState function or not. We currently can't guarantee that for useSelector.

If you're currently triggering all subscribers in the same tick, I don't see how that can avoid incorrect handling due to stale props. It may not actually throw an error, but it might result in some kind of mismatched value in a render before things catch up.

(You're probably right about the "can't have a higher connected ancestor" thing. The vague possibility has been running around in my head for a while, but yeah, I'm not actually sure if it's possible.)

[JeremyRH]

I thought stale props are only caused by the state manager (Zustand or React-Redux) calling a selector after updating the store but before React's render phase.

Asynchronous updates to React's state cause scheduled re-renders but they can be canceled. Example: a parent no longer renders a child in its branch. Even if that child was scheduled to re-render previously, it will not if the parent was scheduled first and did not render the child.

Note: I could be wrong about React's scheduled re-renders. I have only stepped through the code in a handful of different scenarios.

[JeremyRH]

So this goes back to the order of subscribers. As long as a parent is higher than its children in the subscriber order, the child will not be re-rendered with stale props. The selector can still be called by the state manager with stale props, but the try-catch handles that.

[markerikson]

That's the issue. If a selector function uses props inside, it can be run immediately from the subscription before the parent has had a chance to re-render and pass down new props:

https://react-redux.js.org/api/hooks#stale-props-and-zombie-children

It's the selector executing outside the render phase that's the problem.

And as I said, the use of stale props may or may not actually cause an error to be thrown. Totally depends on how the selector logic is written and how the store state is defined. But, the point is there may be at least one execution of the selector with "wrong" values.

[JeremyRH]

there may be at least one execution of the selector with "wrong" values.

Yeah that's true. I know the way this functions isn't ideal but I don't see how it can cause a mismatched value in a render.

[JeremyRH]

Maybe concurrent mode will expose an issue. I know there is this repo that shows issues with state management libraries:
https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-mode