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

Enable initial migrations, clarify README #14

Merged
merged 3 commits into from
Aug 30, 2017

Conversation

IanVS
Copy link
Contributor

@IanVS IanVS commented May 3, 2017

I had a bit of trouble getting started with this library, and this PR attempts to make it a bit easier for others. It does a few things:

  • Allow a version key of 0. Previously, 0 was being turned into -1. I don't think this was intentional.
  • Perform migrations when the user does not have a previously-stored version (i.e. the first time the app is deployed with redux-persist-migrate. This is a fairly substantial change, and I put some notes in the commit message. I'm happy to discuss the pros/cons further.
  • Update the README to clarify that you need to have an existing reducer to store the version in.

IanVS added 3 commits May 3, 2017 12:09
Previously a `0` would be interpreted as falsy, but I believe
the real intent here is to avoid an undefined due to an empty
`versionKeys` array.
Previously, if there was not already a stored version, migrations would
not happen at all.  This change allows _all_ migrations to occur, the
first time redux-persist-migrate is deployed and users have stored
state, but no stored version.

I think this is safe, because the only other realistic time the version
will be null is when persisted data has been purged, in which case there
will be no other `state` to migrate.  

The only other situation I can think of which might cause problems, is 
if the developer has not created a reducer at all in which to store the
version.  In that case, all migrations will be performed, but the new
version will not be saved, so the migrations will occur each time.  I
think the solution to this is to improve the documentation to clarify
that a reducer needs to be created to store the version, and that
`redux-persist-migrate` will not create any reducers.
@pavoni
Copy link

pavoni commented May 8, 2017

This is a great.

Just spend a while fighting the existing reducer issue - and then found that migrations weren't run if you don't have an existing key (i'm moving from an older persist store - so need this).

Would love it merged if possible.

@pavoni
Copy link

pavoni commented May 8, 2017

Only other thought is that I'm using whitelisted reducers, and so needed to add the version reducer key to my whitelist.

Probably obvious that it needs to be persisted - but wasn't in my case not when I was starting out!

@IanVS
Copy link
Contributor Author

IanVS commented Jun 28, 2017

@rt2zz do you have any comments or changes for this PR?

@IanVS
Copy link
Contributor Author

IanVS commented Aug 25, 2017

@rt2zz, if this is too many changes to review at once, I can try breaking it into multiple PRs, if that's easier.

@rt2zz
Copy link
Contributor

rt2zz commented Aug 30, 2017

@IanVS apologize this took so long to get around to. Looks great!

I have been working on redux-persist v5 which includes a concept of migrations built it (basically this lib). It turns out the versioning bug survived all the way through! I just fixed it on v5 now. The main difference is now we enforce the keeping of persist state (version and rehydrated), so creating the migrations is simpler (no need to define setter / getters).

@rt2zz rt2zz merged commit 033d818 into wildlifela:master Aug 30, 2017
@IanVS IanVS deleted the perform-initial-migrations branch August 31, 2017 12:19
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.

3 participants