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

Check the version of the state object #587

Merged
merged 1 commit into from
Apr 9, 2020
Merged

Check the version of the state object #587

merged 1 commit into from
Apr 9, 2020

Conversation

jameskerr
Copy link
Member

@jameskerr jameskerr commented Apr 9, 2020

Drop the previously saved state if the version is different
This is a crude way to handle state migrations. I'd like to
think this through more, but at least there are no startup
errors when you upgrade versions now.

Going forward, we'll need to increment this VERSION string each time we mess with the shape of the state.

I think it would be good to write a test that does some of the common operations on the redux store, the saves the resulting state in a snapshot. Then each time we make changes to the state shape, hopefully that test will fail with a message like "update the state version".

Depending on how critical it is for users to save state, like their history and in the future, their saved queries. We may want to write more code to migrate data and what not.

fixes #584

Drop the previously saved state if the version is different
This is a crude way to handle state migrations. I'd like to
think this through more, but at least there are no startup
errors when you upgrade versions now.
@jameskerr jameskerr changed the title fix: Check the version of the state object Check the version of the state object Apr 9, 2020
@@ -5,7 +5,12 @@ import throttle from "lodash/throttle"

import type {State} from "../state/types"

export const VERSION = "5"
export const VERSION = "6"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version string is just an arbitrary number, not tied to our app versions in any way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the global reducer and the window reducer import this same constant.

import globalReducer, {type GlobalState} from "./globalReducer"

export default function(initialState: GlobalState | void) {
export default function(prevState: GlobalState | void) {
let initState = isCurrentVersion(prevState) ? prevState : undefined
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figure it was best to let each store creation be responsible for dealing with its version.

@jameskerr jameskerr requested a review from mason-fish April 9, 2020 15:51
Copy link
Contributor

@mason-fish mason-fish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, so is the current plan for us to manually update the VERSION if we make a breaking change to the store schema then?

@jameskerr
Copy link
Member Author

@mason-fish Yes, that is the plan for the time being.

@jameskerr jameskerr merged commit 858fc67 into master Apr 9, 2020
@jameskerr jameskerr deleted the state-versions branch April 9, 2020 16:30
@philrz philrz mentioned this pull request May 5, 2020
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.

master starts to error screen with existing space
2 participants