rt2zz / redux-persist

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

_persist clinging #589

Closed adeelshahid closed 6 years ago

adeelshahid commented 6 years ago

Is it possible to remove _persist clinging to every reducer on rehydrate.

rt2zz commented 6 years ago

_persist needs to exist at the level that the persistReducer is applied. This is the only safe way to track state versioning. What issue are you having with it?

adeelshahid commented 6 years ago

@rt2zz I divided my reducer's with a separate config for each for extensibility. Upon rehydration I get back.

_persist: {} _props: {}

I think redux-persist should not amend internal reducer structure by adding props/_persist to it, that's just my opinion.

rt2zz commented 6 years ago

Do you have a suggestion for where else to store that state?

adeelshahid commented 6 years ago

may be storing it internally inside some object which takes care of persist true/false flag.

rt2zz commented 6 years ago

that is possible and something we might be open to, but we need to consider a couple thing:

  1. will the extra read per persistoid add meaningfully to startup time?
  2. will the extra code to store this state outside of the reducer state add to code size?
  3. will the risk of out of sync issues (where state is written but _persist fails or vice versa) going to be problematic?
rohozhnikoff commented 6 years ago

same wishes

i made fabric of nested reducers, and often initial state isnt the {} type new redux-persist api is incredible, and fit good with our composition of reducers but with _persist we got another constraints now we should put persistReducer only to 'plain-object Reducer type' "token" or phone, i have to group them in some needless key and only then persist

rohozhnikoff commented 6 years ago

it's also a huge problem, if reducer is objectOf(shape({})) type normalized collections, which manage by own reducer it makes me filter _persist key from collection on .map, on connect, etc

rohozhnikoff commented 6 years ago

internally, persistReducer run spread and assign at every action

spread is not cheap, particularly in normalized collections with thousands of items most of the js implementations, constructing new {} with loop

assign work in same, wrong for perfomance way and it makes another leak, cause return always new {}, even if nothing was changed

in current solution, we do them every time, in every action-fire and we have to create some better solution

rohozhnikoff commented 6 years ago

and it makes another leak, cause return always new {}, even if nothing was changed

...which mark all the tree-branch like "dirty", and don't give combineReducer to make this https://github.com/reactjs/redux/blob/e6463f2da9020185c6461c5975700f756e506015/src/combineReducers.js#L160

it is wrong input for connect(), who make only shallowEqual diff and think that your unchanged, but 'spread > assign' piece of data is dirty, which cause needless .render() and diff-s

you don't see it, if use only flat version of reducers but if you like to use hierarchy of your state, and serve piece of data closer to branches that have changes - you will use combination of combineReducers (example https://github.com/reactjs/redux/issues/738)

rohozhnikoff commented 6 years ago

basically we could, at least, dont create new {...baseReducer(state, action), _persist} if _persist || baseReducer(state, action), _persist} didn't changed

changes of _persist we could see in code changes of state we could look from reference by state !== newState (usually redux docs recommend to return state, if no action to be handled in reducer)

rohozhnikoff commented 6 years ago

another issues require removing _persist from state do you have any demand-list, for what cases we use _persist ? i would to think about another way

adeelshahid commented 6 years ago

@rt2zz

  1. will the extra read per persistoid add meaningfully to startup time? This only comes into effect if we need to do an extra read. e.g. if configuration specifies that a particular Object is going to be stored per key. Otherwise we can do a direct read, just like configuration per reducer.

  2. will the extra code to store this state outside of the reducer state add to code size? Store object keys inside _docs: [] Storage.get/set on keys inside the _docs reducer with a counter to know when all the keys have been resolved. Set then to a Map/Object and send it back on rehydrate.

  3. will the risk of out of sync issues (where state is written but _persist fails or vice versa) going to be problematic?

Sync is a difficult problem when too much is happening in a single place, current model of redux-persist with [fetch and restore] on load while [set and save] on modification works well.