zustandjs / zustand-slices

A slice utility for Zustand
MIT License
180 stars 8 forks source link

Action merging might lead to unpredictable behaviour #31

Closed unbeauvoyage closed 1 week ago

unbeauvoyage commented 2 weeks ago

In real world applications slices will usually be in different files and there could be dozens of properties (even hundreds, if the app is using a single store), and it might cause name collisions and bugs. There will be higher likelihood of developers being caught off guard and an additional layer they need to be aware of. Since this library already provides withActions() for store level actions, where one can easily compose individual actions anyway, I would humbly suggest that it might be worth reconsidering this approach.

In the original PR where the relevant discussion took place, @dai-shi implied that this behaviour is open for consideration and suggested it could be discussed in a new issue. I take this opportunity to suggest that action merging does not seem to bring enough benefits to justify the risks it carries. Composing two explicitly named actions in a single store level action is safer and easily achievable anyway, like so:

import {createSlice, withActions, withSlices} from 'zustand-slices';

const countSlice = createSlice({
    name: 'count',
    value: 0,
    actions: {
        inc: () => (prev) => prev + 1,
        resetCount: () => () => 0,
    },
});

const textSlice = createSlice({
    name: 'text',
    value: 'Hello',
    actions: {
        updateText: (newText: string) => () => newText,
        resetText: () => () => 'Hello',
    },
});

export const useCountStore = create(withActions(withSlices(countSlice, textSlice), {
    resetAll: () => (state) => {
        state.resetCount()
        state.resetText()
    }
}))

Originally posted by @unbeauvoyage in https://github.com/zustandjs/zustand-slices/issues/30#issuecomment-2379338014

dai-shi commented 2 weeks ago

Thanks for opening a good discussion.

Do you think of any good case with the current behavior? (assuming, the moment after people get used to it.)

The rationale is the current action is similar to Redux's reducers. Multiple reducers do some jobs for a single action. (But, then Redux toolkit hides it with extraReducers, so it's probably too hard for human being.)

unbeauvoyage commented 2 weeks ago

Yes, I think the above rationale is a valid one and indeed good a use case. But, as you implied, redux toolkit's abstraction does not compromise predictibality. One can use extraReducers and can still be explicit enough about what's going on when actions are dispatched, which is not the case in zustand-slices' current action merging approach.

The current approach introduces a hidden layer of complexity that may lead to unexpected bugs, particularly in larger applications with many slices. Explicitly composing actions, like in the example in the issue intro, would provide a safer and more predictable alternative. And zustand-slices already offers that.

I cannot think of a real-world use case where composing actions with "withActions()" would be harder than merging actions or defining extraReducers (if zustand-slices had such an api).

dai-shi commented 2 weeks ago

Yeah, that's fair, predictability is more important. As I said, I don't dislike the idea if the implementation gets simpler.

Composing two explicitly named actions in a single store level action is safer and easily achievable anyway

This is actually convincing. When I first introduce the action merging idea, we didn't have withActions.

dai-shi commented 2 weeks ago

any thoughts? @grigorischristainas @EskiMojo14 @Ebubekir-Tas @grumd

EskiMojo14 commented 2 weeks ago

i agree, action merging was actually one of my main concerns with the library.

on the RTK side of things, we don't have any implicit merging of slice actions because they're all namespaced to avoid collision, so as discussed you explicitly need to use extraReducers to choose to respond to the same action (which you should do, to be clear)

grigorischristainas commented 2 weeks ago

That's a good discussion indeed! I understand the concerns about predictability, especially in large-scale applications. Composing individual actions using withActions seems to be a flexible and straightforward alternative.

dai-shi commented 1 week ago

I was hoping to get feedback for some more people, but I guess this is the consensus. Predictability fits better with Zustand.

Thanks guys for the suggestion.