Wrong migration typings

[filipjnc]

There's something wrong with the MigrationManifest typings. The way it requires to define a migration is as follows:

export const migrations = {
  0: (state: PersistedState) => {
    return { ...state, activeId: state.activeId };
  },
};

However I get an error that activeId does not exist on PersistedState.

I also tried assigning state: PersistedState & State, which solves the error in the migrations file but then complains in the createMigrate function, since it accepts exclusively PersistedState.

A solution would be to make PersistedState take a subtype like this: PersistedState<State>, which will be imported from the reducer.

[lafiosca]

The relevant typings are...

createMigrate: https://github.com/rt2zz/redux-persist/blob/d7efde9115a0bd2d6a0309ac6fb1c018bf06dc30/types/createMigrate.d.ts#L14

Which takes a MigrationManifest: https://github.com/rt2zz/redux-persist/blob/d7efde9115a0bd2d6a0309ac6fb1c018bf06dc30/types/types.d.ts#L75-L77

Which is a dictionary of functions which take and return PersistedStates: https://github.com/rt2zz/redux-persist/blob/d7efde9115a0bd2d6a0309ac6fb1c018bf06dc30/types/types.d.ts#L9-L11

This seems odd to me for multiple reasons. For one, PersistedState can be an object containing a _persist field OR it can be undefined. But based on my reading of the createMigrate code, it doesn't seem possible that the undefined will ever be passed into the MigrationManifest functions because if state is undefined, the function returns early here: https://github.com/rt2zz/redux-persist/blob/d7efde9115a0bd2d6a0309ac6fb1c018bf06dc30/src/createMigrate.js#L16-L20

So it already seems PersistedState is too broad for the definition of the input of these functions, as they should never receive undefined as input. But even if we ignore the undefined possibility, e.g. by converting the typing to this:

  interface MigrationManifest {
    [key: string]: (state: Exclude<PersistedState, undefined>) => Exclude<PersistedState, undefined>;
  }

we'll still have the problem mentioned in the OP above.

The next reason the typing seems odd to me is that my app and migrations don't care at all about the _persist slice of the state, which is the only thing PersistedState specifies. Why should the end user's MigrationManifest type be concerned with that? The _persist slice is metadata used by the migration library logic, not part of the data being migrated.

The final reason that the typing seems odd, and perhaps ultimately a point that makes this difficult to type whatsoever, is that every step of the migrations by definition will be working on differently structured data. Let's envision a simple example with 3 versions of a root state: https://gist.github.com/lafiosca/a992fa57dc2bb871108fbfc1ef1ad93e

Each of these migration steps (moving from version 0 to version 1, moving from version 1 to version 2) has very specific, explicit shape structures that we can define types for. It would be nice if the createMigrate argument typing could support this, although it seems to me that it would quickly become unwieldy to manage a generic type in an attempt to handle every version's interface. I think the simplest approach may be to loosen up the typing, something like:

type MigrationManifestItem<T extends {} = {}, U extends {} = {}> = (state: T) => U;

interface MigrationManifest {
  [key: string]: MigrationManifestItem<any, any>;
}

An alternative approach which might give slightly more type safety would be to use a generic which expects a union type of every possible version state interface:

interface MigrationManifest<T extends {} = {}> {
  [key: string]: (state: T) => T;
}

export default function createMigrate<T>(migrations: MigrationManifest<T>, config?: MigrationConfig): PersistMigrate;

From our simple example, we'd then do this:

type MigrationState = RootStateV0 | RootStateV1 | RootStateV2;
const migrate = createMigrate<MigrationState>(migrations);

(If we don't explicitly state the MigrationState generic parameter, TypeScript will infer RootStateV0 and complain about the other shapes.)

I'm willing to work up a PR if the library maintainers have a preference on how this should be improved. I am unsure if any of these would be considered a breaking change, although I have trouble imagining that anyone is successfully using createMigrate in TypeScript without some form of type coercion already. I've tested the latter approach locally and will likely use it via patch-package in my own project for now.

[lafiosca]

I've created a gist delineating the approach I ended up taking: https://gist.github.com/lafiosca/b7bbb569ae3fe5c1ce110bf71d7ee153

[lafiosca]

Ironically right around the same time @rvonzimmerman was doing the PR above, I was working on a new migration in my app and found that even my approach seems insufficient for certain types. I haven’t had time to research it deeply yet, but my union type was causing a type inference conflict after I added another version to my migrations. Apparently just removing the explicit union type from the createMigrate call cleared it up, but I didn’t have a chance to examine it deeply.

[rvonzimmerman]

@lafiosca Sorry I jumped the gun throwing up the PR. I'll keep a fork for myself and stay in tune to see if this gets merged, the typings are unusable right now.

Could you share the issue you're facing with them right now? I'd like to take a look!

[worldlee78]

So yeah, I too have been having this problem, and the typings are definitely wrong. For instance, the documentation says that you can return either the state, or the Promise of a state. Which doesn't seem to be captured in the migration typings here at all.

I'm curious, has anyone been able to successfully do Typescript and Migrations? If so, how are they doing it now? I've been patching my migration file with @lafiosca suggestion, but that also has it's own drawbacks it seems.

[lafiosca]

Thanks @worldlee78, I didn't realize that they could return Promises. I certainly did not account for that in my approach.

@rvonzimmerman I think the problem I'm encountering with this approach is that the using the union type parameter MigrationState for the MigrationManifest causes it to expect every migration function to accept and return the entire union type.

That is to say, for example:

const persistMigrations: {
    2: (state: PersistedRootStateV1) => PersistedRootStateV2;
    3: (state: PersistedRootStateV2) => RootState;
}

Argument of type '{ 2: (state: PersistedRootStateV1) => PersistedRootStateV2; 3: (state: PersistedRootStateV2) => RootState; }' is not assignable to parameter of type 'MigrationManifest<RootState | PersistedRootStateV2 | PersistedRootStateV1>'.

And that makes sense. Because if I'm passing my union type MigrationState as the type parameter to this:

interface MigrationManifest<T extends {} = {}> {
    [key: string]: (state: T) => T;
}

Then it means I expect every one of the migration functions to be of the form (state: MigrationState) => MigrationState, which is to say (state: PersistedRootStateV1 | PersistedRootStateV2 | PersistedRootStateV3) => PersistedRootStateV1 | PersistedRootStateV2 | PersistedRootStateV3. And that's not true.

What I was intending originally was to distribute that union type so that it yielded something more along the lines of:

(state: PersistedRootStateV1) => PersistedRootStateV1
 | (state: PersistedRootStateV1) => PersistedRootStateV2
 | (state: PersistedRootStateV1) => PersistedRootStateV3
 | (state: PersistedRootStateV1) => PersistedRootStateV1
 | (state: PersistedRootStateV2) => PersistedRootStateV2
 | (state: PersistedRootStateV3) => PersistedRootStateV3
 | (state: PersistedRootStateV1) => PersistedRootStateV1
 | (state: PersistedRootStateV2) => PersistedRootStateV2
 | (state: PersistedRootStateV3) => PersistedRootStateV3

But even if that were possible, it would be an overshot of what I ideally want:

{
    2: (state: PersistedRootStateV1) => PersistedRootStateV2 | Promise<PersistedRootStateV2>;
    3: (state: PersistedRootStateV2) => PersistedRootStateV3 | Promise<PersistedRootStateV3>;
}

I don't think there's a way to express what I want to generically in TypeScript. Off the top of my head, the only approach I can think of that might make sense here is to have the library use any or unknown, and then we TypeScript users can explicitly define our MigrationManifest types like this if we want additional type-checking. I am certainly open to suggestions.

[lafiosca]

I think I have gotten a little closer to a working solution, although still not ideal:

  type MigrationFunction<T extends {}, U extends {} = T> = T extends infer V ? U extends infer W ? (state: V) => W | Promise<W> : never : never;

  type MigrationManifest<T extends {}> = Record<string, MigrationFunction<T>>;

  export default function createMigrate<T extends {}>(migrations: MigrationManifest<T>, config?: MigrationConfig): PersistMigrate;

This gives that fully distributed union of functions going from any state to any state like I described above.

Then, again, explicitly specify the union type of all versions of state when creating:

type MigrationState = PersistedRootStateV1 | PersistedRootStateV2 | PersistedRootStateV3;

const persistMigrations = {
    2: (state: PersistedRootStateV1): PersistedRootStateV2 => ({ /* ... */ }),
    3: (state: PersistedRootStateV2): PersistedRootStateV3 => ({ /* ... */ }),
};

export const persistMigrate = createMigrate<MigrationState>(persistMigrations);

This works without TypeScript complaining, but a couple downsides of this approach:

1) Type inference won't work automatically on the createMigrate call. If you don't explicitly provide the union type, you get an error like this:

Argument of type '{ 2: (state: PersistedRootStateV1) => PersistedRootStateV2; 3: (state: PersistedRootStateV2) => RootState; }' is not assignable to parameter of type 'Record<string, (state: {}) => {} | Promise<{}>>'.
  Property '2' is incompatible with index signature.
    Type '(state: PersistedRootStateV1) => PersistedRootStateV2' is not assignable to type '(state: {}) => {} | Promise<{}>'.
      Types of parameters 'state' and 'state' are incompatible.
        Type '{}' is not assignable to type 'PersistedRootStateV1'.
          Type '{}' is missing the following properties from type 'Pick<PersistedRootStateV2, ...

2) It's an overshot, as I explained in the previous comment. This would allow things that don't make sense, like having the migration for version 2 migrate from StateV3 to StateV1.

You could explicitly define your own MigrationManifest type, e.g.:

interface MigrationManifest {
    2: (state: PersistedRootStateV1) => PersistedRootStateV2;
    3: (state: PersistedRootStateV2) => PersistedRootStateV3;
}

But then if you set the type of persistMigrations to that, it will conflict with the createMigrate typing.

So all in all, I still don't see a perfect solution. There are pros and cons to looser or tighter approaches on the typings.

[lafiosca]

I've revised my gist to include more than one migration, additional comments, and the newer patch I've described above: https://gist.github.com/lafiosca/b7bbb569ae3fe5c1ce110bf71d7ee153

Note that this type definition also includes Promise support.

[mirezko]

@lafiosca hi, I am also trying to figure out the typings for MigrationManifest. Have you come up with a solution to this? As I understand the PR was not merged...

[lafiosca]

@mirezko I'm still using the approach in that gist I shared on Feb 14. It mostly works for me, but it's a pretty awkward solution. I didn't think it would make sense to submit as a PR for general use, for the reasons I discussed above. But if people would find it more useful than what's currently in the library, I could try to submit a PR with those caveats.

[PymZoR]

Any news on this ? Looking at the code people are writing (https://github.com/search?l=TypeScript&o=desc&q=import+%7B+createMigrate+%7D+from+%27redux-persist%27%3B&s=indexed&type=Code) it seems that pretty much everyone went the way any => any or @ts-expect-error

[CyxouD]

It is still the issue

[oleksandr-danylchenko]

Still struggling with the any => any migrations