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

Having Rehydrated values passed in the store's initialState #21

Closed
zalmoxisus opened this issue Oct 22, 2015 · 22 comments
Closed

Having Rehydrated values passed in the store's initialState #21

zalmoxisus opened this issue Oct 22, 2015 · 22 comments

Comments

@zalmoxisus
Copy link
Contributor

I still looking into it, but it would be great to achieve. Now the reducers are initiated twice: the first one with the initial value, and the second - with the rehydrated one. I tried to find some solutions, but all of them seems to be antipattern, except of this. Actually, this is the purpose of the store's initialState to pass the rehydrating state from server or localStorage.
What do you think if we get the data from the localStorage before creating the store?

@rt2zz
Copy link
Owner

rt2zz commented Oct 22, 2015

I thought about this, and am definitely interested in supporting this, but upon initial inspection I found two issues:

  1. If you are using async storage, you then have to create the store in your callback, which means (in react) you have to delay rendering the redux Provider component. Achieving which is much more complex than simple syncronous intialization.
  2. Some rehydrations require processing, such as immutabljs transforms or access token validation. In these cases I like having an explicit debuggable action to understand how the state went from initial -> rehydrated. Furthermore I think it is valuable that reducer's can opt in to their own rehydration.

It would seem the key to supporting this use case of initialState rehydration would be to provide a lower level getStoredState function that can be used in lieue of autoRehydrate.

What do you think?

I think the value of redux-persist is that it provides a common extensible interface for rehydration, and in that vein / the spirit of redux, I would like to support the maximum use cases with the minimum api surface.

@zalmoxisus
Copy link
Contributor Author

Yes, I also found having initial -> rehydrated state transformation very useful for debugging and do want to use it for access token validation as well, but it has some drawbacks:

  1. UI flickering, as we render according to the initial state, and then rerender with rehydrated state.
  2. We still need a big bounce, otherwise the initial states are stored before rehydratation.
  3. Tests with selenium webdriver on CI often fail as they catch initial values instead of rehydrated ones.

Having an asynchronous lower level getStoredState function for the initialState would be a solution, but we should avoid repeated rehydratation after initialization in this case.

@zalmoxisus
Copy link
Contributor Author

Well, those issues can be solved by just returning the store asynchroniously in the persistStore callback.
Also crosstabSync should be called after the persistStore is completed, otherwise the initial value is synced (I'll do a pull request with this change in the README).

The downsides of this approach are that the rendering is slower (but with getStoredState it would be almost the same) and the states are still stored in the storage again (because they are changed from the initialState). A solution to the latter could be to move store.subscribe to the rehydrationComplete, but I'm not sure how it suits #12. What do you think?

@rt2zz
Copy link
Owner

rt2zz commented Oct 24, 2015

I have only partially formed thoughts on these very good points you raise:

  1. For specifically the sync localStorage case, we could avoid UI Flickering by calling the localStorage getItem syncronously invoking the localStorage callbacks syncronously (instead of using setImmediate as it currently does). I wrote this assuming async storage API's, but maybe it was a mistake to force asyncronicity with setImmediate.
  2. Initial state should never be stored regardless of debounce. The action buffer in autoRehydrate should ensure that rehydrate runs first before any other actions that might trigger a storage. That said I just realized action buffer was broken because of the change in compose behaivor in redux@2, I just published a fix for that in 1.2.0.

I also added a debounce config parameter (in in versions >=1.1.0). From my initial test debounce: false works great.

It has occured to me that crosstab-sync implicitly depends on the fact that string equality exists in the serialized state post-rehydration. This may not be the case in chrome.storage since it stores unserialized objects. I have not fully thought this through, but we may need to abandon shallow merge in autoRehydrate in order to preserve post-rehydration object equality.

@rt2zz
Copy link
Owner

rt2zz commented Oct 24, 2015

Also regarding the question at hand or initialState, one possible api for this could be

import { getStoredState, storeStateOnChange }

//...
const store = createStore(reducer, getStoredState())
storeStateOnChange(store)

The problem is, getStoredState would only work as described with a sync api, namely localStorage. I am not sure any elegant way to do this with an async storage api.

@zalmoxisus
Copy link
Contributor Author

Thanks a lot for the changes! I'll look into it.

I like the current approach of using async storage API, and see 2 big advantages for a web app:

  1. Easy to extend to React Native or to a Chrome extension.
  2. We can use localForage, which will increase the performance a lot by replacing localStorage with IndexedDB/WebSQL where it is possible.

Regarding initialState, we shouldn't enforce any promise library, so it would be only one solution - to get the store in a callback:

export default function configureStore(callback) {
  getStoredState(initialState => {
    const store = createStore(reducer, initialState)
    storeStateOnChange(store)
    callback(store)
  })
}

And then use it like that:

import React from 'react'
import { render } from 'react-dom'
import Root from '../containers/Root'
import configureStore from '../store/configureStore'

configureStore(store => {
  render(
    <Root store={store} />,
    document.getElementById('root')
  );
});

I'm still not sure whether we need it if the previous issues are solved.

@rt2zz
Copy link
Owner

rt2zz commented Oct 24, 2015

Would the above snippet (react rendering in an async callback) not also cause selenium testing issues?

I definitely like where you are going with this, and I think there are a (minority) of projects that will find this approach better suited. Code wise implementation will be trivial, just need to expose existing functionality via two new methods.

Since this has turned into a full blown api review I have a few questions:

  1. storeStateOnChange is wordy, I would like something more elegant. Unfortunately persistStore is already taken :)
  2. Should debounce default to false (currently defaults to 33ms)
  3. Should serialization be the responsibility of the storage engine? Or should it be a boolean serialize: false. It seems obtuse to have to set serialize and deserialize to () => {}.

@zalmoxisus
Copy link
Contributor Author

It suits well selenium tests as we already wait till the page is loaded/rendered. Actually, it was possible before to add an additional waiting while we get the necessary value, but if it fails, it would be difficult to figure out what caused the issue.

  1. Maybe we can improve persistStore instead of adding another function. If getStoredState was called, it should act as storeStateOnChange was intended to act.
  2. What is the purpose of adding a debounce interval?
  3. A solution would be to use localForage by default instead of localStorage, which does the serialization if it is needed. Honestly, I didn't try it, but it seems to increase the productivity, and I do not see any drawbacks (if we specify another storage, it should be removed by Webpack as a dead code).
    I created a library which adds those obtuse parameters and storage, they are used only when we have chrome.storage, otherwise we use default values along with persist-crosstabs. I added you as a coauthor BTW :)

@rt2zz
Copy link
Owner

rt2zz commented Oct 24, 2015

  1. That saves the need to pass in config twice which is great. Still I need to think on this a bit more.
  2. Debounce is necessary for performance sesitive tasks, such as writing to storage while scrolling. Nevertheless I agree, this is the minority case, so lets default false.

@rt2zz
Copy link
Owner

rt2zz commented Oct 31, 2015

Ok just pushed an update with a new getStoredState method. I will publish npm in the future after adding more tests and doing some code cleanup.

Here is the gist:

import { getStoredState, persistStore } from 'redux-persist'

const config = {
  skipRehydrate: true
}

getStoredState(config, (err, initialState) => {
  const store = createStore(reducer, initialState)
  persistStore(store, config)
  render(
      <Root store={store} />,
         document.getElementById('root')
      </Root>
  )
})

Of course this can be split up essentially the same as in your comment, except with an error first callback and shouldRehydrate: false in config.

Overall I am pretty happy with this approach, and the symmetric config makes things easy. Also interestingly it opens up the possibility for secondary persistence schemes. e.g.

// main persistor using localForage
let mainPersistor = persistStore(store)
let remotePersistor = persistStore(store, {storage: secondaryServerStorage, shouldRehydrate: false})

This could trivially enable cool features like observing user sessions - just hook up an instance of your app to the remotePersistor (identified by some user id).

@zalmoxisus
Copy link
Contributor Author

Just tested, that's awesome!

The only problem I see, if we have skipRehydrate: true, crosstab-sync wouldn't be able to rehydrate as well. Or am I doing something wrong?
I guess you meant this parameter in the second gist for the secondary persistence schemes.

@rt2zz
Copy link
Owner

rt2zz commented Nov 1, 2015

@zalmoxisus ah yes, that was an oversight. A lot of ambiguous terms floating around, if you have suggestions for more explicit naming I am all ears.

For now I will change skipRehydrate -> skipRestore, and then allow ad hoc rehydration through (e.g. crosstab sync). Also I will add a couple more tests and likely publish npm.

@rt2zz
Copy link
Owner

rt2zz commented Nov 2, 2015

renamed config to skipRestore, now works with adhoc rehydration (read: crosstab sync).
https://github.com/rt2zz/redux-persist#semi-secret-advanced-apis

@rt2zz rt2zz closed this as completed Nov 2, 2015
@zalmoxisus
Copy link
Contributor Author

@rt2zz, thanks a lot for the changes, but it still doesn't work. Adding autoRehydrate store enhancer in this scenario, blocks action dispatching at all. Removing the condition here helps, though we do not need a REHYDRATE_COMPLETE action dispatching here.

@rt2zz
Copy link
Owner

rt2zz commented Nov 2, 2015

facepalm I added tests on the persistStore side, forgot about autoRehydrate. Will fix asap

@rt2zz
Copy link
Owner

rt2zz commented Nov 2, 2015

persistStore now emits REHYDRATE_COMPLETE even if skipRestore:true

While inelegant, this is the only non-error prone way I can see to preserve the action buffer (which is very useful in a naive implementation). Actually emiting a completion action is useful, the problem is it should be renamed to something more accurate, since no rehydration occured.

@rt2zz
Copy link
Owner

rt2zz commented Nov 2, 2015

note there is still one gotcha, if you do not call persistStore, the autoRehydrate actionBuffer will never be released. I will try to address this in 2.0.

@zalmoxisus
Copy link
Contributor Author

Thanks a lot!

Another advantage of this new approach is that now redux-persist is compatible with redux-devtools' persistState (before the history was reset on the initial rehydratation). It is the only case where having REHYDRATE_COMPLETE is not preferable as it appears in history on every page reload, and it could be ambiguous, as rehydratation didn't happen in this case (the storage value could be different as the initialization didn't happen). Since it is just for debugging and not so many people use debug_session, we could leave it as an enhancement request.

@rt2zz
Copy link
Owner

rt2zz commented Nov 3, 2015

👍

I have not used redux-devtools much as I am mostly in react-native atm.

@zalmoxisus
Copy link
Contributor Author

@rt2zz, is it ok to not include autoRehydrate store enhancer at all if I use the current scenario and don't have hydratations from such libraries as redux-persist-crosstab?

@rt2zz
Copy link
Owner

rt2zz commented Nov 8, 2015

@zalmoxisus yes, in fact it is preferable not to use autoRehydrate in this case.

@ufukomer
Copy link

ufukomer commented Jun 20, 2017

@rt2zz What is the solution for immutable.js type states? I'm trying this solution with redux-persist-transform-immutable but it gives error about type mismatch. #21 (comment)

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

3 participants