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

V5 everything under single local storage key #489

Closed
mauron85 opened this issue Oct 16, 2017 · 6 comments
Closed

V5 everything under single local storage key #489

mauron85 opened this issue Oct 16, 2017 · 6 comments

Comments

@mauron85
Copy link

mauron85 commented Oct 16, 2017

It seems that redux-persist-transform-filter now store everything under single key.

const entitiesSubsetFilter = createFilter(
  'entities',
  ['deliveries']
);

const config = {
  storage,
  key: 'tracker',
  whitelist: ['entities'],
  transforms: [entitiesSubsetFilter]
};

Whole state is persisted in localDb under single key: persist:tracker = { entities: { deliveries: {} }}

In V4 separate key was created for every reducer:

reduxPersist:entities = { deliveries: {} }, and so on.

I think V4 is more performant as only slice of state will be serialised on state change (V4) vs serialising whole state atom (V5).

image

On screenshot V5 key is highlighted. Other keys are from V4

EDIT:

Also as seen on screenshot {"entities":"{\"deliveries\":{}}"} are unnecessarily double escaped.
It seems that redux-persist is doing serialisation by calling JSON.stringify twice. This is also new in V5 (in V4 it was ok).

V5 seems to be doing

JSON.stringify({ entities: JSON.stringify({ deliveries: {} }) });

V4 seems to be doing

JSON.stringify({ entities: { deliveries: {} } });

which will persist key's value as: {"entities":{"deliveries":{}}}

@rt2zz
Copy link
Owner

rt2zz commented Oct 16, 2017

@mauron85 thanks for the feedback. Ill explain the logic behind it, but yes there is probably room for further optimization, I am very open to new ideas on the matter.

Why v5 works like it does
By using just 1 key, we get a couple of big benefits:

  1. no longer need getAllKeys in storage adapter. This means rehydrate is faster, and makes the lib more compatible with storage libraries that lack a decent version of this method.
  2. state is stored all at once so you can never end up with inconsistent state atoms (e.g. if there is an implicit dependency between two sub reducers, it could be problematic if one updated and the other fails or is interrupted.
  3. v5 supports nested persisted reducers. This means we can no longer reasonably assume we are persisting the top level combineReducers reducer - and in turn for handing this power back to the implementor, it is less clear how to automatically break up the persistence.

Performance:
The double stringify comes from a performance "optimization" we have, which is that we serialize each key separately over time. Stringify is typically the most costly part of persistence (and is all on the main thread) so we want to make sure to break that up as much as possible. Saving to disk however is typically on another thread and async so, saving a giant combined value all at once is not so scary.

The double stringify then comes from the fact that we have a bunch of sub-states serialized, that we need to combine and serialize again before we can save to disk. I have done some very basic benchmarking - JSON.stringify of a stringified object is 100% - 150% faster than JSON.stringify of the original object. IMO worth the the tradeoffs.

That said there is probably room for further optimization. e.g. since we know we are always storing an object of strings at the top level, perhaps there is a more compact and custom way to do the final serialization that does not require escaping the quotes in the sub state strings.

@mauron85
Copy link
Author

Ok, it make sense to me now. Specially point 2. about state inconsistency. Also I think double stringify is not problem for now (but maybe can be improved like you said).

What I don't understand is point 3. - nested persisted reducers. Can you give me some example what are those?

@rt2zz
Copy link
Owner

rt2zz commented Oct 16, 2017

in v5 this would work:

let rootReducer = combineReducer({
  a: persistReducer(persistConfigA, a),
  b: persistReducer(persistConfigB, b),
})

so e.g. you might persist one set of state to sessionStorage and another to localForage. Or you might configure special rules in one to expire stored state on rehydration etc.

@mauron85
Copy link
Author

mauron85 commented Oct 16, 2017

That is actually pretty cool. In my react-native project I actually need to use two stores. One for small data and another one for quite large data.

@BordekarDarshan
Copy link

in v5 this would work:

let rootReducer = combineReducer({
a: persistReducer(persistConfigA, a),
b: persistReducer(persistConfigB, b),
})

@rt2zz Can you plz tell me how to do this

@gitaudhub
Copy link

Raising this issue again,

For the most part I understand the improvements that were mentioned by @rt2zz, though I'd like to better understand the benchmarking done on the JSON.stringify() mentioned. Do you have a code example to help me better understand the test done?

Now, the reason I'm raising this issue again is because I want to emphasize two things we are losing when adding escaping characters or doing a double stringify.

  • We are losing a lot of space.
    • considering that chrome has a 5MB limit on the LocalStorage, every additional character is wasting that space.
  • We are losing devtools functionality,
    • when inspecting the local storage in devtools, if the object is stringified once, you can see it as a regular object and change its fields with ease while testing
    • but when we double stringify devtools will assume the data is just another large string, and will display one long line of characters.

Regarding the points for mentioned about V5

  • Point 2 advocates that all reducers should be stored as one store in the local storage to avoid inconsistencies in reducers.
    • while this is a great point, I think this should be always given as an option to the dev
  • on point 3 you actually give that option to the developer, but I don't know if under the hood you still write everything at once.
    • So I am not sure if it is a contradiction.
  • My personal opinion is, if we always write the entire store to the local storage every time we make a small update, and we have to stringify it twice
    • it will wast both processing power and storage capacity
    • note that some objects can get quite large and small updates to a small reducer shouldn't cause a full store rewrite in the LocalStorage (not sure if thats currently the case)
    • local storage is set as a key value pair just like redux store has reducers as "keys" and their corresponding "data". and it seems to me we can benefit from using it similarly in the local storage.
    • if a developer knows that two reduces must always be in sync. let them just apply the same reducer1PersistConfig and in it they can specify, bundle option or something.
    • if they have just one persist config for the entire store, they can choose again bundle or not to bundle.

These are just a few opinions, but as always, they are just opinions, please let me know what you think.

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

4 participants