Refine v1.0 APIs

[gsklee]

A few thoughts:

1. I don't see any reason to use this library with only persistState but not mergePersistedState, so it doesn't make sense to make persistState as _the_ default export. Please consider making it work like this instead:

   import {persistState, mergePersistedState} from 'redux-localstorage';

2. It's not good to import something from a third-party library's inner directory structure because this creates an extra layer of code dependency. This will cause breaking changes when you simply wanna change your project's directory structure. For reference, React is also moving all the addons out of the main package into independent packages.
3. It feels really weird that in case I have to include a filter (which is gonna be very common for real world cases), my related code would go from simply:

import {compose, createStore} from 'redux';
import {persistState} from 'redux-localstorage';

compose(
  persistState()
)(createStore)

To this bloated mess:

import {compose, createStore} from 'redux';
import {persistState} from 'redux-localstorage';
import adapter from 'redux-localstorage/lib/adapters/localStorage';
import filter from 'redux-localstorage-filter';

compose(
  persistState(
    compose(
      filter('nested.key')
    )(adapter(window.localStorage))
  )
)(createStore)

The weirdest part is - I don't understand why we have to create the localStorage storage ourselves. Shouldn't this be handled by the library which is called precisely redux-localstorage?

If there is no justified benefit, couldn't we make it a bit simpler like this?

import {compose, createStore} from 'redux';
import {persistState, localStorage} from 'redux-localstorage';
import filter from 'redux-localstorage-filter';

compose(
  persistState(
    compose(
      filter('nested.key')
    )(localStorage)
  )
)(createStore)

Where the localStorage export is simply adapter(window.localStorage).

[gsklee]

PS. The adapter pattern is good, but I think it should remain invisible for officially supported storages (ie. localStorage, AsyncStorage, etc.). End users should only be required to construct a storage using adapter only when trying to fit an unsupported storage into the lib.

[elgerlambert]

Hi @gsklee, thanks for opening an issue! As I’ll elaborate on below I’m faced with a bit of a dilemma with the direction the v1.0 API is heading which touches some of your remarks, so I appreciate your feedback.

I’d like to share the consideration I made in relation to the points you’ve made, not to defend them, but to ensure we’re on the same page.

1. Some might prefer to have each reducer contain the required logic to rehydrate their corresponding part of persisted state. If so, mergePersistedState should not be used and instead the various reducers can listen for redux-localstorage’s INIT action. (More info)
2. You’re referring to importing an adapter, correct? You make a valid point, exposing the project’s structure this way isn’t great for the reason you mention. My reason for this approach is to prevent the other adapters from being included into your codebase since you’ll only need one -which (to my knowledge) is what would happen if I included all the adapters as exports to index.js. You could argue this highlights the problem of including the adapters as part of this package.. this touches your third remark.
3. The functionality that the filter storage enhancer provides was part of the api/package for the majority of the releases for much the same reason you mention; it’s a common thing to do/require. There were two reasons that motivated me to move it to a separate repo despite that.
   1. Although it’s currently still based on the old api, redux-localstorage-slicer demonstrates there are interesting alternatives to reach the same result. An alternative that in this case has the added benefit of being able to decide which parts should/shouldn't be persisted during runtime. I didn’t want to “bless” the filter implementation I had created by including it in the package.
   2. It also became clear to me that having storage enhancers be part of this package wasn’t sustainable due to the potential diversity and the number of version bumps required due to adding/fixing/changing them.
4. The number of pieces that are required to get real utility from this package has increased as the 1.0 API has evolved and that’s something that’s been bothering me (the bloated mess you refer to). The issue that’s been raised in #16 and the best solution I’ve been able to reach (in my mind) thus far (https://github.com/elgerlambert/redux-localstorage/pull/16#issuecomment-146509511), further add to this concern.

I find myself agreeing with your remark that it seems strange that you require an adapter for localStorage given the name of this package. In that sense I like the api you propose in your last code snippet. What’s important to realise though is that you would be required to include a storage enhancer to serialise/deserialise (in other words JSON.stringify/parse) your store state. (This is exactly the same consequence that the proposed solution for #16 has, but in this case you get something back in return, which is that you don’t require an adapter anymore). It could look something like this:

import {compose, createStore} from 'redux';
import {persistState, localStorage} from 'redux-localstorage';
import filter from 'redux-localstorage-filter';

const storage = compose(
  serializer(JSON.stringify, JSON.parse),
  filter('nested.key')
)(localStorage)

const store = compose(
  persistState(storage)
)(createStore)

The above snippet illustrates something else I’ve been thinking about, which is that quite a few use cases for storage enhancers simple want to transform the state in some way, creating a storage enhancer for each seems a little verbose. An interesting alternative might be to simply create a transform storage enhancer that applies an Array of transform functions. I’d have to look into it further, but this could potentially also make it easier to use e.g. lodash-fp’s version of _.pick to replace redux-localstorage-filter.

Apologies for the amount of info to digest, might have to split this discussion up into a couple of issues, but we'll see how it goes. Like I said I'd appreciate your input/feedback. cc @slybridges, @vicentedealencar, @ruprict, @eisisig, anyone else reading this.

[gsklee]

A more generic transform storage enhancer sounds like a good idea. I'd also like to point out that

compose(
  serializer(JSON.stringify, JSON.parse),
  filter('nested.key')
)(localStorage)

is simply just

serializer(JSON.stringify, JSON.parse)(
  filter('nested.key')(localStorage)
)

So if we're willing to sacrifice a little bit of performance we can reverse the order of application

filter('nested.key')(
  serializer(JSON.stringify, JSON.parse)(localStorage)
)

And simply do export const localStorage = serializer(JSON.stringify, JSON.parse)(window.localStorage) in the package, simplifying user code to

compose(
  filter('nested.key')
)(localStorage)

[slybridges]

I like the transform idea too. I also like what @gsklee demonstrated, having default exports so that the most basic use doesn't need setting any enhancer/serializer/transformer.

[elgerlambert]

Might not have understood correctly, but by removing the need for an adapter (for storage backends with an API similar to localStorage), why would this package still include an export for said storage backends?

I actually fully agree with the sentiment that it would be great to apply the JSON stringification step by default (that's why it's currently part of the adapter), but doing so gives rise to the issue described in #16.

It's possible to apply the JSON stringification step as part of the defaultStorage that's assigned internally if you don't provide a storage argument to persistState (thus making the most basic use easier). If you provide your own storage instance however, (because you want to e.g. persist only a subset of store state), then I don't see any other way then that developers have to ensure that the state is stringified themselves. That said, if the transformState storage enhancer discussed above becomes part of the package (further research pending), then it's only a matter of adding JSON.stringify and JSON.parse as transforms, which might not be so bad.

Thoughts?

[elgerlambert]

I've done some work on the generic transformState enhancer to feel it out. It's signature is currently as follows tranformState(down, up), whereby the arguments down and up both accept an array of transformation functions. This enables the following:

import _ from 'lodash-fp'
import {compose, createStore} from 'redux';
import persistState, {transformState} from 'redux-localstorage';

const storage = compose(
  transformState([
    _.pick('todos'),
    JSON.stringify,
    btoa
  ], [
    atob,
    JSON.parse
  ])
)(localStorage)

This looks pretty good to me. It solves the current issue with encryption. It also makes it easy to use lodash-fp functions such as _.pick and _.omit which are more generic and versatile than the number of custom storage enhancers that have been created to solve the same need. It also reduces the pain of having to JSON.stringify/parse the state yourself.

EDIT: _.pick and _.omit unfortunately don't support nested.keys... there doesn't appear to be a method to get the subset of an object directly. Would still be possible through a combination of _.get and _.set for example, but that's not really great; requires too much effort.

[gsklee]

@elgerlambert Looks cool, if I'll suggest something it'd be that the signature of transformState can be transformState({up, down}) so by looking at the code alone I can tell right away which part defines the "up" transformation and vice versa, without looking at the doc.

[elgerlambert]

@gsklee like this?

const storage = compose(
  transformState({
    down: [
      _.pick('todos'),
      JSON.stringify,
      btoa
    ],
    up: [
      atob,
      JSON.parse
    ]
  })
)(localStorage);

[gsklee]

Yep.

[elgerlambert]

In my eagerness to simplify, I overlooked something important in my reasoning.

In order to support asynchronous storage backends (such as React Native’s AsyncStorage), the way I see it, localStorage will have to be wrapped by some sort of an adapter; either to call a callback or to return a promise (or an observable) instead..

[tjallingt]

Hey, I was wondering if there was any progress on improving the new API?

[elgerlambert]

Hi @tjallingt,

The recently released rc5 has taken a lot of inspiration from what has been discussed in this issue. It includes the transformState enhancer proposed above.

Furthermore, the default adapter export still includes JSON (de)serialisation, but now it's also possible to access the adapter without (de)serialisation applied to it which provides a solution for the issue that was raised in #16 with regards to encryption.

There's one suggestion that hasn't been incorporated into rc5 and that's @gsklee's suggestion to expose adapter(window.localStorage) directly as localStorage. The argument here is to maintain consistency & an attempt to enforce the idea that you can pass in any persistence layer you want.

Did you have any feedback or things that were discussed above that you would "+1" or were you more curious when 1.0.0 would (finally) be published as latest on npm?

[tjallingt]

Hey @elgerlambert thanks for the reply,

I was mostly interested in when the release of the changes as described above would be.

As for my two cents on exporting localStorage: I understand the need for consistency and agree with the argument that adapter(window.localStorage) is clearer overall. I'd suggest shipping this package with only the localStorage adapter and putting the others in their own redux-localstorage-adapter-[storage-backend] packages. This would make the "default" usecase look like:

import persistState, { mergePersistedState, adapter } from 'redux-localstorage'

On another note: although i understand your reasoning for keeping filter in its own package i dislike that decision because it makes this package much more difficult to use. For example I have a lot of trouble telling what the following section does:

const storage = compose(
  filter('playlist')
)(adapter(window.sessionStorage));