Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enhanceReducer can not be used with more than 1 slice #9

Open
johannesloetzsch opened this issue Jul 27, 2023 · 3 comments
Open

enhanceReducer can not be used with more than 1 slice #9

johannesloetzsch opened this issue Jul 27, 2023 · 3 comments

Comments

@johannesloetzsch
Copy link

First of all, thanks a lot for this nice library, I very appreciate it :)

Till now it is not supported, that more than one slice of the same store can be synchronized.
If one tries do do so, enhanceReducer is called with action.type === SET_STATE_FROM_YJS_ACTION for each reducer.
The current implementation of enhanceReducer causes the state of slices that should not be affected to be overwritten with action.payload.
A solution would be substituting this line of code with a merge of the existing state with the update received from yjs. I will provide a Pull Request doing this…

johannesloetzsch added a commit to johannesloetzsch/redux-yjs-bindings that referenced this issue Jul 27, 2023
@lscheibel
Copy link
Owner

Hey, thanks for taking the time, but as it stands I don't feel comfortable merging the PR.

There are two main reasons:

  1. The change is at best a hotfix that will duplicate state and would probably cause a lot more trouble than I can anticipate right now. As it stands, the bind function should only be called once per store (this might change in the future). If you need this behavior today, you'd have to create one common slice like "sharedState" that then contains multiple "sub"-slices. I recognize however that this goes against the spirit of the library of not requiring any changes to existing Redux stores.
  2. As you've correctly observed, binding to the same store twice with different slice names will behave not as expected because the set state action does not let the reducers know which slice is meant to be updated. A more sophisticated fix would be to expand the payload type of the action that is dispatched, when updating the Redux store, to include the slice name. This would then also allow us to use Redux middlewares instead of the awkward enhanceReducer trick.

Regardless, I'd much rather move to a more generic and flexible way of interacting with the Redux store, which would require the user to supply a getter and setter for said store. At which point, this library is actually not far from being completely Redux agnostic.

If you feel comfortable, I'd be happy to help you out with implementing that :)

@johannesloetzsch
Copy link
Author

Thanks for the fast response and your explanations :)

Yes, I like the idea of adding the slice name to the action. I can try to implement that…

Your suggestion of using getters and setters instead of slices sounds interesting too 👍

@johannesloetzsch
Copy link
Author

@lscheibel Are you aware of https://github.com/SebastianStehle/yjs-redux ?
Looks promising too :) What is your opinion?

…I'm still willing to contribute. Just didn't find the time till now…

During the last months I have been happy using this modification: https://github.com/FormsWizard/processing/blob/main/packages/react-redux-yjs/features/enhanceReducer.ts
It assumes the data of slices to be within an object with a key having the same name as the slice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants