compatibility with redux-loop

omnidan / redux-undo

:recycle: higher order reducer to add undo/redo functionality to redux state containers

MIT License

2.91k stars 188 forks source link

compatibility with redux-loop #107

Closed DavyDeDurpel closed 8 years ago

DavyDeDurpel commented 8 years ago

I can't get redux-undo working in combination with redux-loop (and I guess any other store enhancer might cause troubles). The issue is that using the loop method inside the undoable wrapped reducer returns an array containing the state and another object that is recognized by redux-loop. Redux-undo however stores this array inside 'present' and that breaks the code later on.

I only have a few months experience with React Native so I might not see the complete picture yet but I guess only 2 solutions exist:

or I modify all my logic to check for that array and act accordingly (but that messes up my code ;-) )
or a new callback is added to redux-undo that is called the moment the state is set as the present state (which would allow us in this case to only do something like: 'return state[0];')

What do you think?

omnidan commented 8 years ago

Do you have an example project to reproduce this issue?

DavyDeDurpel commented 8 years ago

Not yet. I did some more tests yesterday and the conflict might also be related to redux-act. Anyway, I will make a sample project to demonstrate the issue.

peteruithoven commented 8 years ago

I'm not sure how redux-loop affects the state, but wouldn't this problem basically apply to every store enhancer / higher order reducer that affects the state?

DavyDeDurpel commented 8 years ago

ReduxUndo107 - working.zip ReduxUndo107.zip

I created 2 example projects. The one with '- working' does not contain redux-undo. The other one is my attempt to integrate redux-undo.

The example is very simple. It shows a number and a button to increase the number. When pressing the number a 'counting' message is displayed for about a second and then the counter is increased. In the non working version I also log the state. It clearly shows that after pressing the button state.present becomes the 'loop'.

I already tried rewriting my mapStateToProps function so that the props are filled in correctly but it doesn't work as the 'loop' does not get executed.

DavyDeDurpel commented 8 years ago

I managed to fix the issue in what I might even consider a good way :-) I basically added support for 'redux-loop' inside your reducer logic. It's a small change and shouldn't break any existing code.

I attached the modified reducer logic so you can have a look at it.

reducer.zip

UPDATE: I haven't fully tested my change. I only tested the insert and not the undo, redo... UPDATE2: undo an jumpto are working; filters do not work (yet)

DavyDeDurpel commented 8 years ago

I needed to do another fix to get the filters working again...

reducer.zip

omnidan commented 8 years ago

@DavyDeDurpel sorry for the late reply - your examples are both the same and neither of them use redux-undo (only a small difference in package.json).

dan@galaxy ~/Downloads> diff ReduxUndo107 ReduxUndo107\ -\ working/
Common subdirectories: ReduxUndo107/android and ReduxUndo107 - working/android
Common subdirectories: ReduxUndo107/app and ReduxUndo107 - working/app
Common subdirectories: ReduxUndo107/ios and ReduxUndo107 - working/ios
diff ReduxUndo107/package.json ReduxUndo107 - working/package.json
9,12d8
<     "babel-preset-es2015": "^6.13.2",
<     "babel-preset-react": "^6.11.1",
<     "history": "^3.0.0",
<     "immutable": "^3.8.1",
14a11,12
>     "history": "^3.0.0",
>     "immutable": "^3.8.1",

Can you try reuploading the example with redux-undo?

I'm pretty sure that if redux-loop sticks to redux principles (pure reducer function), it should be possible to combine them without any changes in redux-undo. It's still hard for me to tell what exactly the issue is, though.

DavyDeDurpel commented 8 years ago

I think something is wrong with the way you do the diff because redux-undo is definitely a dependency in the ReduxUndo107.zip file.

I don't think that redux-loop breaks the 'pure reducer function' principle. They just encapsulate the state in an array and add some meta data to it to indicate what should happen next.

Anyway, I think that I'm going to switch to redux-saga to do my asynchronous calls. That library doesn't touch the reducers themselves so it should never interfere with any other store enhancer or middleware.

DavyDeDurpel commented 8 years ago

I finally switched to redux-saga which is 100% compatible with redux-undo.