Zombie child, yet again

[ratoi-crysty]

There still seems to be the issue of zombie child. It requires some special criteria to reproduce:

1. The state needs to have initial value
2. The delete action, needs to be async called in regards to the event handler (Ex.: wrap the action call inside a setTimeout)
3. This is reproducible only on the elements that were present in the initial state.

Note, that the same scenario is not reproducible while using redux with hooks.

Repo to reproduce: https://github.com/ratoi-crysty/zustand-zombie-child

[dai-shi]

Thanks for the repro.
This seems the same as what we discussed in #286.
The quick fix is: https://codesandbox.io/s/fast-shadow-12726?file=/src/components/example/list.tsx:631-660

Note, that the same scenario is not reproducible while using redux with hooks.

This, I'm not sure why though.

[ratoi-crysty]

Yup! It seemed related to that and as you pointed out in your codebox, using unstable_batchedUpdates, will do the trick.

But I still have one question though.

What would be the downsides of moving the subscription from the useEffect to directly in the hook call? (Ex.: index.tsx)
This way, the listeners will be added in the same order as the component tree render order, therefore, avoiding having the child listener called before the parent. In this case, it should dramatically reduce, if not eliminate, the problem with the zombie child, as the parent will be rendered before the child.

[ratoi-crysty]

As for the scenario when using react-redux. It seems that they are already using it. In the subscription file (Subscription.js) they are using batch to wrap the all the listeners calls. By default, the the batch function is unstable_batchedUpdates. This is set in index.js

[dai-shi]

What would be the downsides of moving the subscription from the useEffect to directly in the hook call?

That would be a side effect in render, which is never recommended in react.
In legacy mode, it would work, but in concurrent mode, it will be really problematic.
It may subscribe to the store and never unsubscribe from it, in some scenarios.

[ratoi-crysty]

Thanks for your answer. It makes sense.

As for the scenario when using react-redux. It seems that they are already using it. In the subscription file (Subscription.js) they are using batch to wrap the all the listeners calls. By default, the the batch function is unstable_batchedUpdates. This is set in index.js

Maybe we should add the functionality to allow passing a batching method when creating a store, this way, it should prevent the zombie-child issue and also improve performance as it will call all listeners of that store inside one batch (if that is done outside the react event, like setTimeout). And having it optional, it will not add any breaking changes.

What's your opinion on this?

[dai-shi]

As for the scenario when using react-redux. It seems that they are already using it. In the subscription file (Subscription.js) they are using batch to wrap the all the listeners calls. By default, the the batch function is unstable_batchedUpdates. This is set in index.js

Ah, nice finding. I thought I saw this before, but I was searching batchedUpdates today and nothing found. 😓

[dai-shi]

We could try using batchedUpdates, but it'd be not easy to make bundles for react-dom and react-native. I wonder if v4 #160 will solve this issue eventually...

Oh, setBatch is totally opt-in. I missed it. That might work. Would you like to work on it?

But, then, the DX is not very good, isn't it? The workaround in #286 seems easier to me.

[ratoi-crysty]

After giving further thoughts, I think that having a setBatch will probably not add allot of value. Because, if you manually wrap your action (in my example, deleteItem) inside the batching method, that will automatically wrap all the listeners calls inside it, as zustand will do this synchronously. And therefore, have the same value as a setBatch method. Another way to it, is to manually wrap the set method in the store creator and only use the wrapped method to update the state, this will also have the same effect as setBatch.

What do you think? Do you see any value in implementing a setBatch? Probably something that I have missed?
If not, then you could close the issue, as it works as expected.

[dai-shi]

Yeah, that's what I meant by "DX is not very good". I don't think it's worth the effort, at this point. But let us keep this in mind.

For now, it would be best for us to add a small section about the workaround in readme.md. Would you or anyone like to work on it? cc @martonlanga @Tirzono .

(I will close this issue a bit later.)

[ratoi-crysty]

@dai-shi let me know if this is ok or if I should change anything.

[drcmda]

one question @dai-shi didn't @JeremyRH find a way to do this without batchedupdates? i remember he told me we can now remove it. the trouble is mostly that we're using two reconcilers: react-dom and r3f.

our middleware currently looks like this:

import { Renderer } from 'react-three-fiber'
import { unstable_batchedUpdates } from 'react-dom'

const batched = (config: StateCreator<BuerliState>) => (
  set: SetState<BuerliState>,
  get: GetState<BuerliState>,
  api: any,
) => config(args => unstable_batchedUpdates(() => Renderer.batchedUpdates(() => set(args))), get, api)

if there is any way to avoid this we would be very happy ...

btw react automatically batches in blocking and concurrent mode, has anyone tested if the problem persists?

[JeremyRH]

To fix this in non-concurrent mode without batchedupdates, you need to ensure a parent's setState is called before its children's setState. There are many ways to do that. I did it by keeping track of render order (mutating an array during render) and it didn't work in concurrent mode.
Someone should be able to figure out a solution but it might involve doing different things in concurrent mode.