State migrations for hydration and loading files #843
Conversation
Codecov Report
@@ Coverage Diff @@
## master #843 +/- ##
==========================================
+ Coverage 70.52% 70.67% +0.14%
==========================================
Files 201 202 +1
Lines 6555 6557 +2
Branches 1103 1106 +3
==========================================
+ Hits 4623 4634 +11
+ Misses 1918 1909 -9
Partials 14 14
Continue to review full report at Codecov.
|
src/redux/migration.ts
Outdated
| * @returns state, mutated | ||
| */ | ||
| export function migrateEachYear(state: YearsTaxesState) { | ||
| for (const year of enumKeys(TaxYears)) { |
There was a problem hiding this comment.
Your as unknown as YearsTaxesState dance in the other file is needed because you're returning a YearsTaxesState type from this function, but you're passing in a subtype of YearsTaxesState, (actually a union of YearsTaxesState).
So I think this could be either:
const migrateEachYear = <S>(state: S & PersistedState & YearsTaxesState): S & PersistedState & YearsTaxesState => ...
Or better:
const migrateEachYear = <S extends PersistedState & YearsTaxesState>(state: S): S => ...
Could this be a new state object created here instead of mutating (it's the redux way right?), and written more simply as an expression, instead of a for statement, for example something like:
enumKeys(TaxYear).reduce((acc, year) => ({...acc, [year]: ...}), {} as S)
We only have a couple for statements in this code base and I think they should be rewritten too.
There was a problem hiding this comment.
I don't see it needing any reference to PersistedState for this function specifically (it doesn't care about that part of state at all). But the rest of it sounds great.
src/redux/store.ts
Outdated
| '0': (state) => { | ||
| // this type dance is unfortunate, but createMigrate isn't typed generically... | ||
| const migrated = migrateEachYear(state as unknown as YearsTaxesState) | ||
| return migrated as unknown as typeof state |
There was a problem hiding this comment.
I bet you can get rid of this. I agree I don't like the type in redux-persist could probably be better, but it's just requiring that the parameter and return is a subtype of PersistedState. So you can enforce any union type of PersistedState in the migration functions, see comment in migration.ts.
There was a problem hiding this comment.
Found a much cleaner way by dropping their whole system for a plain async function with just our simple logic.
src/redux/fs/FSReducer.ts
Outdated
| @@ -21,7 +23,7 @@ const fsReducer = <S, A extends AnyAction>( | |||
| case 'fs/recover': { | |||
| return { | |||
| ...newState, | |||
| ...action.data | |||
| ...migrateEachYear(action.data as unknown as YearsTaxesState) | |||
There was a problem hiding this comment.
Could probably get rid of this double cast by restricting the type of S to extends YearsTaxesState & PersistedState as above
|
@zakpatterson I think the latest commit addresses all that feedback. Let me know if you'd prefer to have it rebased. |
|
Nice find on the utilitytype |
What kind of change does this PR introduce? (delete not applicable)
This uses the built-in migrations feature of
redux-persistto migrate the state shape to the latest whenever it is hydrated. (I believe the initial code from #130 may have never worked.) It also uses a shared function to do the same when loading from a JSON file so older files will continue to function with upgrades to the app.The migrations feature of
redux-persistdepends on a versioning scheme which is a bit overkill for where this app is at the moment, but I think it will be useful in the long run. We just need to be vigilant about bumping the version and including a migration step for each change to the state interface.(Also fixes a typo in the menu.)
Maintainer note:
Fixes an error thrown if localStorage is missing parameters currently expected, due to version updates. Reproducing on master:
Will throw an error after switching to Health Savings Account tab.