ctrlplusb / easy-peasy

Vegetarian friendly state for React
https://easy-peasy.dev
MIT License
5.03k stars 190 forks source link

why are the same migrations run multiple times even though the version is already set #912

Open proohit opened 4 months ago

proohit commented 4 months ago

Hey and thanks for easy-peasy, it has simplified our state management significantly!

I am trying to use the persisted migration feature, but am having trouble to use it securely.

Given the following scenario, it appears that previous migrations are being executed, even though their version is exceeded.

migrations: {
  migrationVersion: 1,
  1: (state) => {
    state.test = "test";
  },
},

Running this migration results in:

{"_migrationVersion": 1, "test": "test"}

After that we add second migration and modify the first one (for the sake of demonstration that is).

migrations: {
  migrationVersion: 2,
  1: (state) => {
    state.test = "test2"; // <-- added 2
  },
 2: (state) => {
    state.test2 = "test2";
  },
},

Running these results in:

{"_migrationVersion": 2, "test": "test2", "test2": "test2"}

As shown, the first migration also runs, even though it has already ran before, resulting in a faulty state. Imagine doing migrations that mutate data based on the previous state, e.g. merging a firstName and lastName to a fullName, which would be done again after the first run, resulting in wrong data or even errors (because firstName and lastName may not exist anymore).

I would have expected that only migrations between the last and the specified version are executed.

jmyrland commented 4 months ago

hey @proohit !

Migrations should work as you expect them to, i.e. they should not run previously executed migrations, as you can see from this test.

I'm not sure why this is not the case in your example.

I added a temporary test based on your example:

test('ISSUE 912', () => {
  // ARRANGE
  const result = migrate(
    { _migrationVersion: 1, test: 'test' },
    {
      migrationVersion: 2,
      1: (state) => {
        state.test = 'test2'; // <-- added 2
      },
      2: (state) => {
        state.test2 = 'test2';
      },
    },
  );

  // ASSERT
  expect(result.test).toBe('test');
  expect(result._migrationVersion).toBe(2);
});

This test passes.

Are you able to reproduce your issue in a sandbox?

proohit commented 4 months ago

@jmyrland will check later if I can provide a reproducer.

Maybe important to note, our environment is react native + mmkv as the storage.