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

Add EncodedLocalStorage #16

Closed
wants to merge 1 commit into from
Closed

Add EncodedLocalStorage #16

wants to merge 1 commit into from

Conversation

slybridges
Copy link

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

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

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

@slybridges
Copy link
Author

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

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

eisisig commented Oct 8, 2015

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

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 elgerlambert mentioned this pull request Oct 24, 2015
@elgerlambert elgerlambert mentioned this pull request Nov 2, 2015
@elgerlambert
Copy link
Owner

elgerlambert commented Jun 26, 2016

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!

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

Successfully merging this pull request may close these issues.

None yet

3 participants