rt2zz / redux-persist

persist and rehydrate a redux store
MIT License
12.93k stars 866 forks source link

Any way to use redux-persist with immutable Map as the entire state? #64

Closed peterlazar1993 closed 7 years ago

peterlazar1993 commented 8 years ago

Want to integrate redux-persist with this https://github.com/multunus/react-native-boilerplate. Maybe a recipe for immutable state

rt2zz commented 8 years ago

I am interested in supporting top level immutable state. You can JSON.stringify a immutable Map to a plain JSON string correct? In this case all we have to worry about is transforming the raw object back to a Map on rehydrate and also how to take the entire state and map it out into per-reducer chunks.

I believe this will require a new option mapStateToPartials or something like that. There will probably need to be corresponding changes throughout the code base.

After typing this out, I believe it is possible to implement with no impact on performance, but it will take some work so no ETA at this time, although PR's are appreciated. If you are short on time I would probably recommend looking at https://github.com/michaelcontento/redux-storage which deals which avoids dealing with state partials and hence has a much simpler story around immutable support.

zalmoxisus commented 8 years ago

If we omit autorehydrate, it should be easy implemented with Alternate Usage

getStoredState(persistConfig, (err, initialState) => {
  const store = createStore(reducer, Immutable.fromJS(initialState))
  persistStore(store, persistConfig)
  // ...
})
rt2zz commented 8 years ago

that works very well for rehydration, but there is still an outstanding problem where persistStore expects the state object returned from getState to be a plain object. A trivial albeit non-ideal performance solution would be to call Immutable.toJS(store.getState()) inside of our store subscriber.

zalmoxisus commented 8 years ago

I guess if we do toJS every time, then we lose the performance gained by Immutable.js, and better not use it at all, unless we persist only some reducers.

peterlazar1993 commented 8 years ago

I see, not entirely related to this issue but does using an immutable structure as the top level state provide any value?

zalmoxisus commented 8 years ago

@peterlazar1993, I think no signifiant value. Better to use the standard Redux combineReducers and use Immutable.js on the reducer basis.

Then we'll have something like

getStoredState(persistConfig, (err, initialState) => {
  const initialImmutableState = {};
  Object.keys(initialState).forEach(function(key) {
   initialImmutableState[key] = fromJS(obj[key]);
  });
  const store = createStore(reducer, initialImmutableState)
  persistStore(store, persistConfig)
  // ...
})

@rt2zz, I guess in this case we don't need any changes in redux-persist. The condition will work and JSON.stringify will transform the state. I'll test it later as I'm still on the way on converting an app to Immutable.js.

peterlazar1993 commented 8 years ago

connect from react-redux implements a shouldComponentUpdate, I think there might be some value in having a top level immutable state. For now, I've decided to use Immutable structures on a per reducer basis.

zalmoxisus commented 8 years ago

@peterlazar1993, it shouldn't matter there even if we don't use Immutable.js at all. Just make sure not to mutate the state, so the references of the state object are different (prevStoreState !== storeState).

peterlazar1993 commented 8 years ago

@zalmoxisus Sure. Thank you :)

rt2zz commented 8 years ago

@zalmoxisus interesting to hear you are moving your sub reducers to immutable, what spurred this change? Is it a performance optimization or that the api is preferable?

Yes if you go with the approach in https://github.com/rt2zz/redux-persist/issues/64#issuecomment-188184873 that example should work without modification. I agree with @zalmoxisus that in a typical redux app there is no benefit to top level immutable state. Keeping things an object at the top level helps with interoperability.

An alternative and simpler option to the getStoredState approach is to use https://github.com/rt2zz/redux-persist-immutable. Which approach is best will be application specific.

peterlazar1993 commented 8 years ago

@rt2zz I've used https://github.com/rt2zz/redux-persist-immutable, neat little library :)

zalmoxisus commented 8 years ago

@rt2zz, you remember I was against it before :) I am still convinced that for most of cases we don't need it, but it gains performance for react components when having lots of sub-instances. Also worth to mention that I persist only few reducers, so serializations will not have signifiant impact on the performance.

zalmoxisus commented 8 years ago

Just an update, in case someone will use the snippet I provided above.

Since getStoredState doesn't take whitelist/blacklist into consideration, you should use the following if don't want to persist all reducers:

import { fromJS } from 'immutable';
import { getStoredState, persistStore } from 'redux-persist';

const whitelist = ['reducerName'];
const persistConfig = {
    skipRestore: true,
    whitelist
};
getStoredState(persistConfig, (err, initialState) => {
    const initialImmutableState = {};
    whitelist.forEach((key) => {
      if (initialState[key]) initialImmutableState[key] = fromJS(initialState[key]);
    });
    const store = createStore(reducer, initialImmutableState)
    persistStore(store, persistConfig)
   // ...
});
rufman commented 8 years ago

https://github.com/rufman/redux-persist/tree/feature/hacked-immutable adds a config option to deal with any random kind of root state. I use it to shallow convert an ImmutableJS state i.e. store.getState().toObject(), so that the root state is a JS object that the rest of the library can deal with. The subStates are still Immutable objects, so I use redux-persist-immutable for actual de/serialization.

zalmoxisus commented 8 years ago

@rufman, wouldn't this shallow convert affect the performance? I still don't get what could be the advantage of having the root state in ImmutableJS.

rufman commented 8 years ago

It's not as bad as doing toJS and I haven't seen a huge effect on performance yet. I use redux-immutablejs, using it's combineReducers which creates an Immutable iterable. I haven't yet come across a real good use case for a global immutable state, but I just started that way, so I would need to refactor a lot of stuff. I just started using immutablejs, so I just decided to go all immutable instead of picking and choosing where to use it. But that's more due to my ignorance/lazyness then an actual argument for or against an immutable root state. Honestly, I think the form of the root state shouldn't matter in terms of middleware/store enhancers being able to work with it.

rt2zz commented 8 years ago

in the forthcoming v3 release the createPersistor and getStoredState methods are sufficiently abstracted that I believe we could easily create a redux-persist-immutable module that reimplments persistStore with top level immutable support.

The following extra config/extension will be needed: autoRehydrate: state iterator, state setter createPersistor: state iterator getStoredState: final state transform (js -> Map)

In order to accept these changes in redux-persist however we would need some benchmarks to ensure we are not introducing performance regressions.

rufman commented 8 years ago

@rt2zz I'll look into creating an immutable module and doing some benchmark testing, when I get some free time.

rufman commented 8 years ago

I've pushed an initial implementation for this issue here: https://github.com/rufman/redux-persist/tree/master any feedback would be nice. I'll open a PR one I've created benchmark tests.

https://www.npmjs.com/package/redux-persist-immutable-state is a new npm-module I published that implements all the config hooks added above for ImmutableJS.

Usage:

import { stateIterator, stateGetter, stateSetter, 
         stateReconciler, lastStateInit } from 'redux-persist-immutable-state';

persistStore(state, {
  transforms: [reduxPersistImmutable], 
  stateIterator: stateIterator,  
  stateGetter: stateGetter, stateSetter: stateSetter,
  lastStateInit: lastStateInit
})
kachkaev commented 8 years ago

Would be really nice if the library supported to-level immutable object. This is being used in mxstbr/react-boilerplate, which is one of the most popular boilerplates in React community today. What are the current plans on resolving this issue?

rufman commented 8 years ago

@kachkaev check out https://github.com/rt2zz/redux-persist/pull/113 and see if that suits your needs.

kachkaev commented 8 years ago

Ah, I wish I saw your PR this morning! Ended up with some clumsy solution based on the original library instead and spent so much time on this! I'll try your modified version of the module in the next project and actually hope that your PR will be finally merged by then!

rt2zz commented 8 years ago

Ok tentative immutable support now available at https://github.com/rt2zz/redux-persist-immutable

note I have not actually run this in an app, but the tests pass ;)

cc/ @rufman if you have a moment to check out my PR on your repo, it would be great to make that the main repo and get you added to the npm module as well

rufman commented 8 years ago

@rt2zz I'm planning on doing that this weekend

kachkaev commented 7 years ago

@rt2zz why have you closed this issue? Has there been a PR that solves it?

rt2zz commented 7 years ago

tentative support via https://github.com/rt2zz/redux-persist-immutable v4.0.0-alpha3

redux-persist-immutable is a drop in replacement for redux-persist that has immutable support