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

shallow merge problem in autoRehydrate #50

Closed
baabgai opened this issue Jan 18, 2016 · 8 comments
Closed

shallow merge problem in autoRehydrate #50

baabgai opened this issue Jan 18, 2016 · 8 comments

Comments

@baabgai
Copy link

baabgai commented Jan 18, 2016

I playing around with redux-persist and redux-persist-immutable to store my app state.
The state is a combinedReducer object with immutable data structure down from the second level in the state tree.
persisting the individual immutable subtrees works very nice with the redux-persist-immutable transform. Unfortunately during rehydration the immutable is correctly restored from local storage but the merge into the store object goes wrong.

The problem seems to be the for...in construct in autoRehydrate.js lines 56,57.
for...in iterates over all properties along the prototype chain and in the case of Immutable.js objects the final object that is merged contains all prototype props in the main level (for...in MDN).
Maybe a quick fix could look something like

...
for (var subkey in reducedState[key]) {
    if (reducedState[key].hasOwnProperty(subkey)) subState[subkey] = reducedState[key][subkey]
}
for (var datakey in data) {
    if (data.hasOwnProperty(datakey)) subState[datakey] = data[datakey]
}
subState.__proto__ = data.__proto__;
autoReducedState[key] = subState
...
@rt2zz
Copy link
Owner

rt2zz commented Jan 19, 2016

This definitely looks like a bug introduced by some for in changes we made. I would prefer to stay away from munging the proto. Perhaps if we detect the object is not plain we can just wholesale set the substate instead of doing for in?

@baabgai
Copy link
Author

baabgai commented Jan 21, 2016

Maybe instead of hacking with proto we could just use Object.create instead of the object literal initialization of subState.
Assuming reducedState, subState and data are both objects of the same type the code could look like

...
var subState = Object.create(Object.getPrototypeOf(data))
for (var subkey in reducedState[key]) {
    if (reducedState[key].hasOwnProperty(subkey)) subState[subkey] = reducedState[key][subkey]
}
for (var datakey in data) {
    if (data.hasOwnProperty(datakey)) subState[datakey] = data[datakey]
}
autoReducedState[key] = subState
...

This should equally work for immutable and plain objects and might be a cleaner and more official way to do it I guess.

@rt2zz
Copy link
Owner

rt2zz commented Jan 28, 2016

how about: https://github.com/rt2zz/redux-persist/blob/master/src/autoRehydrate.js#L38

With isPlainObject check, immutable objects would be wholesale copied instead of shallow merged

@rt2zz
Copy link
Owner

rt2zz commented Jan 28, 2016

ok I pushed the isPlainObject change to master let me know if that solves the issue

@baabgai
Copy link
Author

baabgai commented Jan 29, 2016

ok.
After a few more tests I figured that's actually a much better idea since no data is saved on the first level anyhow for immutables, so a shallow merge done like I suggested would actually always overwrite one immutable object with the other one and not merge anything.

@rt2zz
Copy link
Owner

rt2zz commented Feb 10, 2016

After messing with this more and working on 2.0, I think this is the correct approach. Thanks for reporting!

@rt2zz rt2zz closed this as completed Feb 10, 2016
@arilitan
Copy link

I'm running into an issue where one of my reducers is storing state as immutable objects. When I rehydrate the store however, it is saving as a plain object not an immutable one. Any ideas how to fix?

@rt2zz
Copy link
Owner

rt2zz commented Aug 23, 2016

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

3 participants