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

Async init convention #5

Closed tgriesser closed 9 years ago

tgriesser commented 9 years ago

It doesn't appear as though there's a simple way to know whether the store has been mounted from localStorage.

Would it make sense to add a reducer specific to the state of the adapter interaction which contained the current state: whether state has been loaded from the appropriate adapter, whether it has been initialized, if it is in the process of saving, etc.

I'd like to wait until the state is mounted from the adapter before initializing any components in react-native, as I'm storing the navigation state in localstorage.

Also wanted to know what you thought about adding a way to clear / reset the localstorage state on the top level store.

elgerlambert commented 9 years ago

An indication that the app has finished initialising is something I've been thinking about. I thought about adding a property such as initialised: true/false to the store's state via the mergePersistedState reducer, but thought better of it since I don't want to dictate the store's "shape" in that way. I'm concerned this will cause conflicts with how people want to organise their store state. Adding a reducer like the one you describe would suffer from the same problem.

Currently the redux-localstorage/INIT action is absorbed by the mergePersistedState reducer. I'm considering passing this action on (after merging the initial and persisted state) so that other reducers can respond to this action aswel. This would allow you to define your own reducer that tracks whether initialisation has finished or not. I liked the idea of keeping this action private (it's why I added @@ to it since bete3), but you're the second person to raise this issue and this is the best solution I've come up with so far.

I understand why you would want to know whether or not initialisation has finished or not, but could you explain why you'd want to know whether or not it is persisting/saving? And the distinction between data loaded and app initialised?

I don't fully understand your last remark. Do you want to clear any persisted state from localstorage or do you want to reset your redux store state to what it was after initialisation? Again, if you could tell me a bit about your use-case, that would be helpful.

elgerlambert commented 9 years ago

Hi @tgriesser, I pushed some commits today to the 1.0-breaking-changes branch. Redux-localstorage's INIT action is now passed on. That means you can handle it in one of your own reducers to set some sort of flag whether your app has finished initialising. I recommend you import {actionTypes} from 'redux-localstorage'and then check for action.type === actionTypes.INIT.

Please tell me more about your other use cases if they (still) form an issue for you.

tgriesser commented 9 years ago

Sorry for not getting back here, so in my opinion the ideal solution would be to either return a Promise from the createPersistentStore resolving with the store, or take an optional third parameter callback in lieu of a Promise.

const createPersistentStore = compose(
  persistState(/*paths, config*/),
  createStore
)

createPersistentStore(reducer, initialState).then(store => ...)

Though I guess I could deal with the actionTypes.INIT as you suggest, I'll need to check it out and let you know if there are any issues.

but could you explain why you'd want to know whether or not it is persisting/saving?

I guess there isn't a real need of knowing this, you could always swap out the persistence layer as well if this info is needed.

The last issue of clearing persisted state is actually more something which I'd like to see in redux directly, so I opened rackt/redux#658 for that.

elgerlambert commented 9 years ago

Hi @tgriesser, what you're suggesting would tightly couple redux-localstorage to how redux is implemented. It would also change the API that people expect from createStore. It's a little more verbose, but by setting your own initialised flag (through your own reducers) you can quite easily create your own "callback" by doing the following:

const createPersistentStore = compose(
  persistState(),
  createStore
)

const store = createPersistentStore(reducer, initialState)

const unsubscribe = store.subscribe(() => {
  if (!store.getState().initialised) return
  unsubscribe()
  // Store has finished initialising, do your thing!
});

You might already be aware of this, but just to make sure. Redux-localstorage does have measures in place to ensure your app is initialised correctly despite any async-ness. Any action that is dispatched before the app has finished initialising is buffered. As a result, the @@redux/INIT and redux-localstorage/INIT actions will always reach your reducers first. Any actions that were dispatched before the app finished initialising will follow directly after in the order they were originally dispatched. To be clear, @@redux/INIT is fired directly so your store will have the initial state as defined by the initialState argument you pass to createStore and the default values of your reducers from the get-go.

What this means in practical terms is that only actions/actionCreators that depend on state that was persisted might require more manual effort to delay appropriately.

tgriesser commented 9 years ago

Yep - sounds good! Thanks for the explanation!

ruprict commented 9 years ago

Hey @elgerlambert, any chance you can do a new beta npm module with the action change?

elgerlambert commented 9 years ago

Hi @ruprict, I just published the release candidate that includes these changes; npm install redux-localstorage@rc --save.

Please note that the filter storage enhancer has been moved into it's own repo (and therefore needs to be installed via npm separately). redux-localstorage-debounce is now also available.

Out of curiosity, (this goes for you too @tgriesser), do you need this to enhance the user experience or do you actually need to delay (parts of your) app initialisation (given my previous comment). Just wondering how big the latter use-case is and if I might be able to think of something to better support it.. I guess if you're persisting an authentication token you're gonna have to delay all your requests until initialisation finishes..

ruprict commented 9 years ago

Hey @elgerlambert. First off, YAY! Thanks for the publish!

So, my example is just what you mention: I have a JWT kept in localStorage and I can't do anything until I grab it. It's further complicated for us b/c the app is isomorphic, so I have to figure out how I don't do anything on the server and do do stuff on the client (I am hoping your actionTypes help me here).

Make sense?

ruprict commented 9 years ago

Also, if I npm install as you say, my require("redux-localstorage") fails, as it can no longer resolve the module. I am sure this is due to my not really knowing npm well enough, but a nudge in the right direction would be helpful..... :)

elgerlambert commented 9 years ago

Hmm, while I was typing my comment I was also thinking about isomorphic apps and was wondering what that would look like..

Could you give me more info on what/how it fails? I couldn't reproduce a problem just now. It doesn't have to do with the fact that the filter storage enhancer has been removed from the package?

ruprict commented 9 years ago

Well, I just get Cannot resolve module 'redux-localstorage' in /directory The installed module doesn't have a lib directory, just a src, which has me suspicious. Doing an npm install in node_modules/redux-localstorage brings the lib dir, and then I just get the filter issue, which I fix with the standalone repo.

ruprict commented 9 years ago

So, I am not seeing the new action type in my reducers....shouldn't I see reduxlocalstorage/@INIT now?

elgerlambert commented 9 years ago

I'm afraid I can't reproduce what you're describing. Perhaps you had a git clone of redux-localstorage in your node-modules, in an attempt to grab the changes I hadn't published yet.. ? But it sounds like you managed to resolve the issue now though, that's the import part!

elgerlambert commented 9 years ago

You should be seeing action.type === redux-localstorage/INIT

elgerlambert commented 9 years ago

As stated in an earlier comment above, I recommend you import {actionTypes} from 'redux-localstorage' and then check for action.type === actionTypes.INIT or something along those lines.

ruprict commented 9 years ago

So, that action never hits any of my reducers. I see the redux INIT and any actions I dispatch. The call to dispatch in persistState in your module fires, but it never reaches my reducers. :(

ruprict commented 9 years ago

OK, so a semblance of progress. If you remember, I am using the redux-devtools (remember unliftDevToolsState? :)) and if I don't do that and don't use the devtools, the action hits my reducers.

ruprict commented 9 years ago

Ugh. Bloody devtools balls them up, tossed them in bin It's working now. In fact, I think the module does what I need it to do without the action change (meaning, 1.0.0-beta3 seems to work) Sorry for not turning those off sooner.

elgerlambert commented 9 years ago

Thanks @ruprict for finding that out! Like I said previously I still have to look into integration with devTools more closely, however.. it looks to me as though you can avoid all the issues with devTools if you simply place persistState above devTools, like so:

const createPersistentStore = compose(
  persistState(storage, 'my-storage-key'),
  devTools(),
  createStore
)

With the above setup there doesn't appear to be a need for unliftDevToolsState either. As you can see though I haven't added devTools' persistState to the equation yet.. don't think that should matter though.

@ruprict can you reproduce what I'm saying, does placing persistState above devTools solve the issue and remove the need for unliftDevToolsState as well?

ruprict commented 9 years ago

It seems to, yup. That's great.

elgerlambert commented 9 years ago

Hi @tgriesser, @ruprict. The latest release that's just been published to npm (1.0.0-rc3) now includes a callback to help better solve this issue, more info: readme & https://github.com/elgerlambert/redux-localstorage/commit/b03e08eb5accf7689fb1860d7316a59e1e515b62

raineroviir commented 9 years ago

I tried using 1.0.0-rc3 and I got finalReducer undefined in the console.log, 1.0.0-rc seems to work though

ruprict commented 9 years ago

So, fwiw, using redux 2.0 and the new syntax, the state doesn't seem to get hydrated the same or at the same timing. In other words, now when I initially check state, it hasn't been hydrated from localStorage. Before moving to redux 2.0, the state was already hydrated before any of my code ran. Am I meant to pass in a final callback to persistState so I can then dispatch actions based on that? The finalCallback doesn't take any args, so not sure how I'll get the state in the callback.

Or, am I meant to wait for the INIT action and then dispatch actions based on that? If so, I am not sure how to do that.

There's a great chance I'm just being dumb (again) but a little nudge with how to get the state AFTER it's been hydrated by this module would be helpful to me.

ruprict commented 9 years ago

It might help to point to an example I'm following here, where basically components have a static fetchData function that the route handler will call for "Smart" components. The fetchData method is passed the store and, until I upgraded redux, this store had the state from local storage.

The state in question is "global", meaning, authentication tokens. I don't want to go login again if we have a valid token, and the fetchData function was checking for that.

I guess I could connect the Base handler to the store and dispatch the auth token fetch from it's componentWillMount. Is that better? Suggestions?

ruprict commented 9 years ago

Good news! I'm stupid! Yay!

Forgot the reducer bit from the docs in the 1.0 branch. So, key takeaway: if you don't use mergePersistedState, it won't merge the persisted state.

:laughing: :gun:

elgerlambert commented 9 years ago

Hi @ruprict, glad you figured it out! Not great that I had to/felt the need to introduce a breaking change in a rc version bump; sorry about that! Like I said though, glad you found what you needed in the Readme!