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

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

Closed
peterlazar1993 opened this issue Feb 23, 2016 · 26 comments
Closed

Comments

@peterlazar1993
Copy link

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

@rt2zz
Copy link
Owner

rt2zz commented Feb 23, 2016

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
Copy link
Contributor

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
Copy link
Owner

rt2zz commented Feb 23, 2016

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
Copy link
Contributor

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
Copy link
Author

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

@zalmoxisus
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

@zalmoxisus Sure. Thank you :)

@rt2zz
Copy link
Owner

rt2zz commented Feb 24, 2016

@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 #64 (comment) 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
Copy link
Author

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

@zalmoxisus
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

rufman commented Mar 30, 2016

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
Copy link
Contributor

@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
Copy link
Contributor

rufman commented Apr 1, 2016

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
Copy link
Owner

rt2zz commented Apr 10, 2016

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
Copy link
Contributor

rufman commented May 19, 2016

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

@rufman
Copy link
Contributor

rufman commented Jun 2, 2016

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
Copy link

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
Copy link
Contributor

rufman commented Aug 10, 2016

@kachkaev check out #113 and see if that suits your needs.

@kachkaev
Copy link

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
Copy link
Owner

rt2zz commented Sep 16, 2016

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
Copy link
Contributor

rufman commented Sep 16, 2016

@rt2zz I'm planning on doing that this weekend

@rt2zz rt2zz closed this as completed Sep 18, 2016
@kachkaev
Copy link

kachkaev commented Sep 18, 2016

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

@rt2zz
Copy link
Owner

rt2zz commented Sep 18, 2016

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

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

5 participants