elgerlambert / redux-localstorage

Store enhancer that syncs (a subset) of your Redux store state to localstorage.
MIT License
1.32k stars 107 forks source link

Add EncodedLocalStorage #16

Closed slybridges closed 8 years ago

slybridges commented 8 years ago

Adds a local/session Storage that encodes the string before storing it and decodes it after getting it from the storage. Useful if you do not wish to store your state as plain text.

elgerlambert commented 8 years ago

Hi @slybridges, thanks for your PR. To me this seems like an ideal case for a storage enhancer though (as opposed to an adapter). It'll actually be more useful that way, since it can then be used with any storage backend adapter!

It would be great if you published it as a storage enhancer to npm and added an entry to the wiki. Check out the filter and debounce storage enhancers for guidance/inspiration.

elgerlambert commented 8 years ago

p.s. Please don't hesitate to ask me anything if you have questions about converting your adapter to a storage enhancer.

slybridges commented 8 years ago

Thanks for the feedback @elgerlambert.

I actually started this as an enhancer. But problem is that the localStorage adapter does the following in its get method:

callback(null, JSON.parse(storage.getItem(key)));

The decoder enhancer method would need to be called just between storage.getItem(key) (that would return the encoded string) and JSON.parse (that is expecting the decoded string of course).

Hence the separate storage adapter.

I agree that this would be great as an enhancer but, unless i'm missing something, this would require a change of API to the adapters (like adding a callback hook)

elgerlambert commented 8 years ago

Hi @slybridges,

Thanks for explaining, I understand the issue now. It's a bit of a dilemma really..

The adapters are only intended to map various existing storage API's to a single consistent API (so that it can be consumed by redux-localstorage and so that it's compatible with the various storage enhancers). They're not really intended to contain much, if any, functionality. I guess this issue demonstrates the problem when you do..

To that end, it makes the most sense to me to remove the JSON.stringify/parse from the adapter, making it the responsibility of a storage enhancer, which would also enable encoding to be done through a storage enhancer.

The dilemma comes from the fact that this adds another piece to the puzzle that's required to get started. It's also slightly frustrating since the value will always have to be JSON.stringified/parsed at some point in the chain (in the case of local-, session-, and AsyncStorage); feels like losing some utility value, forcing developers to add this step themselves to the chain.

eisisig commented 8 years ago

Just a quick thought, not well thought out... Can't the default value for the encoding/decoding params be a return function so if you want something else you can send it in with the adapter call else it returns the default data

export default (storage, encode = data => data, decode = data => data) => ({});
// Then for those who wish to do something special
adapter(storage, btoa, atob)
elgerlambert commented 8 years ago

Hi @eisisig, thanks for chipping in!

I did have a similar thought. What I dislike about it though, is that it adds more functionality to the adapter rather then less; moving (further) away from it's original intent and blurring the lines between storage adapters and enhancers.

Furthermore, it doesn't make much sense to add these arguments to adapters for storage backends that don't necessarily require stringification (localForage, PouchDB, LevelDB). Thereby leading to different call signatures for different adapters, I fear this will lead to confusion and distracting documentation.

In short, I feel this would take it in a direction that is likely to lead to more issues rather than less.

elgerlambert commented 8 years ago

As proposed in my comments above JSON (de)serialisation has been removed from the actual adapter(s) itself (as of rc5). I invite you to have a look at the code snippet (& note) associated with the transformState enhancer in the Readme for an example of how to solve the issue raised above.

@slybridges although I've decided to solve this in a different way I thank you for your PR and for raising the issue!