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

v3 changelog & RFC #72

Closed
rt2zz opened this issue Apr 3, 2016 · 11 comments
Closed

v3 changelog & RFC #72

rt2zz opened this issue Apr 3, 2016 · 11 comments

Comments

@rt2zz
Copy link
Owner

rt2zz commented Apr 3, 2016

Breaking changes

  • autoRehydrate no longer implement an action buffer. It was too automagical and caused some bugs with dispatch return values. Updgrade path: implement redux-action-buffer

Other changes of note

  • new createPersistor method that handles subscribing to the store and persisting to the storage engine.
  • under the hood persistStore now simply calls getStoredState and createPersistor
  • getStoredState now support promises
  • localForage support through aliasing keys -> getAllKeys
  • createTransform(inbound, outbound, {whitelist, blacklist}) method
  • pass key to transforms (enabling whitelist/blacklist above)
  • add preloaders config which get run before getStoredState and allow for advanced plugins like redux-persist-migration

todo

  • consider "lazy" loading
@corbt
Copy link

corbt commented Apr 19, 2016

Big +1 on migrations, as I mentioned earlier. I'm about to do a backwards-incompatible restructuring of my app state and it's pretty painful as things are right now.

@rt2zz
Copy link
Owner Author

rt2zz commented Apr 27, 2016

Ok this is what I am thinking for migrations:

It can be done via a transform. It might look something like this:

const migrationManifest = {
  0.1: {
    reducerA: (state) => {...state, somethingStale: undefined} 
  },
  0.2: {
    // ...
  }
}

let migrator = createMigration(migrationManifest)

persistStore(store, { preloaders: [migrator.preloader], transforms: [migrator.transform] })

So basically the manifest is an object with state "versions" as keys and objects which map per-reducer state as values.

Notes:

  • I wish we could get semver for state versions, but since we want fast comparisons (i.e. v1 > v2) I think decimals will be more performant
  • migrations will need to be nearly idempotent, since there is a chance we will lose track of the version and need to rerun all migrations on top already migrated state.
  • migrator transform will need to be placed at the top of the stack so it happens after all transforms

Another issue is how to store the state version. I am thinking migrator can store it in a separate storage key reduxPersist:_stateVersion. If we go this route we need the ability to delay transforms until the state version is loaded out of storage. This can probably be achieved via a preloaders config on persistStore.

These are just early thoughts. I am thinking of releasing this as a contrib module first and if it goes well we can look at direct integration.

cc/ @corbt

@rt2zz
Copy link
Owner Author

rt2zz commented Apr 27, 2016

initial implementation: https://github.com/wildlifela/redux-persist-migration

@corbt
Copy link

corbt commented Apr 28, 2016

I don't think semver would help with migration versions. Monotonically increasing integers would actually work just fine, the only important thing is that there's a stable total ordering.

I may not be totally understanding the implementation, but does each reducer inside a migration only get access to its own state? How do you deal with combining/splitting two reducers? For example, I just migrated data in my app from Recordings nested inside Albums to having separate reducers for both. Would this migration strategy support that?

What if you tweaked the API to just look like:

const migrationManifest = {
  0.1: (state) => ({...state, staleReducer: undefined} ),
  0.2: // ...
}

I think it's ok if the implementation is slightly less performant, the migration code should only be called once per device in normal cases.

Also good to know that the migrations may be called multiple times, but I'm curious why that's the case? Might be worth using multiSet in the AsyncStorage implementation which, while probably not totally transactional, makes it likely that the data will stay in sync.

@rt2zz
Copy link
Owner Author

rt2zz commented Apr 28, 2016

ah, so the current implementation is basically a minimal implementation using current functionality (well aside from the small addition of preloaders). I definitely like the whole-state approach you outline above, but it may require some larger api additions.

Regarding running multiple times, the issue boils down to, how do we know the version of the stored state? If storage fails for some reason, the store will end up with initialState and no known version. The migrator will then apply all of it's migrations on the next rehydrate since the version is null. There may very well be a more elegant solution to this, but I have not thought of it yet :)

Anyway, I will keep thinking it over. Let me know if you have any ideas for exactly what the api should look like.

@danfo
Copy link

danfo commented Apr 30, 2016

Migrations, nice one! I like the API.

My approach until now was to have a schema version as the top-level state key, and have my app deal with noticing existing data which needs to be migrated. I tried replacing this in my project today 👍

When you have state and no known version number, that is certainly a challenge... The current behaviour is running all migrations, even against initialState... This is good for existing users, but seems it will break unless initialState remains in a pre-migrated state.

Since I want initialState to be the latest state shape, I guess I want it to just set the latest version number and move on if there is no persisted state. I'll try that for myself now and see how it goes.

@rt2zz
Copy link
Owner Author

rt2zz commented Apr 30, 2016

👍 let us know how that goes. Getting the version number is the biggest challenge in the api design, perhaps keeping it in redux state is the way to go.

@danfo
Copy link

danfo commented Apr 30, 2016

hmm... well, here's my humble preloader:

function setInitialStateMigrationVersionPreloader({ storage }) {
  return new Promise((resolve, reject) => {
    storage.getItem(storageKey).then((persistedState) => {
      if (persistedState === null) {
        storage.setItem(versionKey, latestVersionKey).then(resolve).catch(reject);
      } else {
        resolve();
      }
    }).catch(reject);
  });
}

New users with no persisted state and no migration version: No migrations run

Existing users with persisted state and no migration version: All migrations run

(no migrations run for new users because I'm ensuring they don't end up having persisted state and no migration version)

So end behaviour seems to be ok, but I wonder how we could get this into the lib. I wonder how it would work if we checked migration version on rehydrate transform, and set migration version when state is persisted. Maybe having migration version in redux state would allow something like that.

@rt2zz
Copy link
Owner Author

rt2zz commented Apr 30, 2016

That does solve the re-running migrations issue, but the issue I see is as follows:

has stored state state version result
yes number run migrations
yes null ?
no number do nothing
no null do nothing

There are two ways we can get to the second scenario:

  1. migrations were just added to the config
  2. version key storage failed

for 1) I would say since there was no pre-existing implementation it should be out of scope for migrations to handle this. If needed a developer can write an special custom migration in their reducer.
for 2) I am not sure. Is it better to rerun all migrations and assume they are idempotent, or would it be better to skip migrations and hope there is no conflict. It seems to me that rerun is the safer option.

@rt2zz
Copy link
Owner Author

rt2zz commented Apr 30, 2016

after struggling with the api a bit, I am pretty convinced migration functionality is better of living inside of a store enhancer, similar to autoRehydrate.

Here is a untested poc implementation of a createMigration store enhancer:

Pros:

  1. better manifest api as suggested by corbt
  2. simpler implementation
  3. completely predictable and synchronous as there is no storage interaction
  4. no extra api required in redux-persist

Cons:

  1. requires adding a version key to redux state (? could also be a pro)
  2. api is a bit heavy with specifying a versionSelector and versionSetter
  3. requires proper ordering of store enhancers (i.e. things will fail silently if it is placed after autoRehydrate)
  4. mutates rehydrate action

As for the cons, 1 & 2 I am ok with, and 3 & 4 can be solved later if need be by adding a simple integration point in persistStore.

Overall I am pretty happy with this approach and will publish a release after doing some testing.

@rt2zz
Copy link
Owner Author

rt2zz commented May 5, 2016

released 👍 📦 🎉

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