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

fallback to default version in migrations #546

Merged
merged 1 commit into from
Nov 7, 2017
Merged

Conversation

levsero
Copy link
Contributor

@levsero levsero commented Nov 7, 2017

Related issue/comment: #434 (comment)

What's changed?

Currently if you try and upgrade from v4 to v5 using https://github.com/rt2zz/redux-persist/tree/b85b56a889ebd4919c9ba11d239a4ae1861e1da6#experimental-v4-to-v5-state-migration and also add a migration, the migration will attempt to run on the v4 state which has no _persist version.

This pr would allow the migration to fall back on the default version in such a scenario.

Considerations

  • I do not believe this would affect the code in any other situation as regularly this won't be executed without having the _persist.version.
  • Is there a clearer or cleaner way to tie this code specifically to using the v4 migrate. eg. perhaps we can manually add _persist in the v4 getStoredState so the v4 migration will not affect any other parts of the code?

@rt2zz
Copy link
Owner

rt2zz commented Nov 7, 2017

@levsero interesting, glad you are testing out the v4->v5 state migration! Is it working otherwise?

I think agree with defaulting state version here. Since the code relies on version existing, if it were to not exist for any reason (even though this should never happen) we want the migration to still make sense.

@rt2zz rt2zz merged commit 9aef4d2 into rt2zz:master Nov 7, 2017
@rt2zz
Copy link
Owner

rt2zz commented Nov 7, 2017

actually I just realized the this default to default version if state version === 0 which i incorrect. Will need to update

@rt2zz
Copy link
Owner

rt2zz commented Nov 8, 2017

updated to include an undefined check 👍

@levsero
Copy link
Contributor Author

levsero commented Nov 8, 2017

Good catch on the === 0 👍
Haven't come across any other issues in the v4 -> v5 migration, seems to work as intended.
💯 on all the work you've done here.

@levsero
Copy link
Contributor Author

levsero commented Nov 8, 2017

@rt2zz wondering how soon do you plan on cutting another release? (Would like to make use of this asap in production and would prefer not having to fork anything.)

@rt2zz
Copy link
Owner

rt2zz commented Nov 8, 2017

I cut a release under the next tag (5.3.0-rc) with this change as well as a change to support non-object substate. I would say the changes are fairly safe so if you are able to give it a go and confirm we are 👍 I will cut 5.3.0 proper.

@levsero
Copy link
Contributor Author

levsero commented Nov 8, 2017

@rt2zz can confirm that on 5.3.0 the migrations (both from v4-v5 as well as the state migration) all appear to be working correctly 🙇

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.

2 participants