Page MenuHomePhabricator

[web-db] implement asynchronous `redux-persist` migrations
ClosedPublic

Authored by kamil on Apr 27 2023, 6:58 AM.
Tags
None
Referenced Files
F2085339: D7658.id26383.diff
Sun, Jun 23, 6:57 AM
Unknown Object (File)
Wed, Jun 19, 9:50 PM
Unknown Object (File)
Wed, Jun 19, 6:22 PM
Unknown Object (File)
Wed, Jun 19, 6:45 AM
Unknown Object (File)
Thu, Jun 13, 4:53 PM
Unknown Object (File)
Thu, Jun 13, 7:36 AM
Unknown Object (File)
Sun, Jun 9, 7:17 PM
Unknown Object (File)
Fri, Jun 7, 7:58 PM
Subscribers

Details

Summary

This is createMigrate code allowing to async migrations.

This could potentially be patched, but this code will have more responsibilities in near future (D7668), so it's better to maintain this that way.

Depends on D7289

Test Plan
  1. Run tests
  2. Add one more async migration and check if it works

Tests could not be added directly, this function will read something from local storage (D7668) - that being said this test will fail, but code I used:

// @flow

import { creatAsyncMigrate } from './create-async-migrate.js';

describe('async redux migrations', () => {
  it('should run migrations with async functions', async () => {
    const persistedState = {
      oldKey: 'oldKeyValue',
      _persist: {
        version: 1,
        rehydrated: false,
      },
    };

    const asyncMigrations = {
      [1]: async state => {
        return {
          ...state,
          oldKey: 'oldKeyValue',
        };
      },
      // needs to run
      [2]: async state => {
        return {
          ...state,
          newKey: 'newKeyValue',
        };
      },
      // needs to run
      [3]: async state => {
        return {
          ...state,
          oldKey: 'oldKeyUpdated',
        };
      },
    };
    const migrate = creatAsyncMigrate(asyncMigrations, {
      debug: true,
    });

    const currentVersion = 3;
    const migratedState = await migrate(persistedState, currentVersion);

    expect(migratedState).toEqual({
      oldKey: 'oldKeyUpdated',
      newKey: 'newKeyValue',
      _persist: {
        version: 1,
        rehydrated: false,
      },
    });
  });

  it(`should do nothing when inboundVersion and currentVersion match`, async () => {
    const persistedState = {
      oldKey: 'oldKeyValue',
      _persist: {
        version: 3,
        rehydrated: false,
      },
    };

    const asyncMigrations = {
      [1]: async state => {
        return {
          ...state,
          oldKey: 'oldKeyValue',
        };
      },
      [2]: async state => {
        return {
          ...state,
          newKey: 'newKeyValue',
        };
      },
      [3]: async state => {
        return {
          ...state,
          oldKey: 'oldKeyUpdated',
        };
      },
    };
    const migrate = creatAsyncMigrate(asyncMigrations, {
      debug: true,
    });

    const currentVersion = 3;
    const migratedState = await migrate(persistedState, currentVersion);

    expect(migratedState).toEqual(persistedState);
  });
  it(`should return undefined for undefined persisted state`, async () => {
    const persistedState = undefined;

    const asyncMigrations = {
      [1]: async state => {
        return {
          ...state,
          oldKey: 'oldKeyValue',
        };
      },
    };
    const migrate = creatAsyncMigrate(asyncMigrations, {
      debug: true,
    });

    const currentVersion = 1;
    const migratedState = await migrate(persistedState, currentVersion);

    expect(migratedState).toBeUndefined();
  });
});

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Apr 27 2023, 7:54 AM
kamil edited the summary of this revision. (Show Details)
kamil edited the test plan for this revision. (Show Details)
ashoat added inline comments.
web/redux/create-async-migrate.js
6–12 ↗(On Diff #25832)

Inputs should be read-only

14 ↗(On Diff #25832)

Typo

15–16 ↗(On Diff #25832)

Inputs should be read-only

tomek requested changes to this revision.Apr 28 2023, 4:11 AM
tomek added inline comments.
web/redux/create-async-migrate.js
27 ↗(On Diff #25832)

Is this correct to skip this? I may be wrong, but some redux functions accept State | void and it might be valid for a migration to receive undefined and return something. Not a big issue though, as that shouldn't happen in our case.

51 ↗(On Diff #25832)

In MigrationManifest we allow string as a key. Is it a good idea to sort them like that?

65 ↗(On Diff #25832)

Can this be falsy?

66 ↗(On Diff #25832)

What is the plan for handling exceptions here? Do we want them to be just thrown which would mean that all the migrations failed?

This revision now requires changes to proceed.Apr 28 2023, 4:11 AM

fix typo and make inputs read-only

@tomek questions you added are valid but those are questions to the redux-persist author. I wanted to copy-paste code from source code to preserve old logic, as this returns function which will be used inside redux-persist logic. I only updated from the sync function which returns a promise - to implicitly return a promise from the async function (which will allow us to put await before any migration).

Any additional modification could cause some undefined behaviors, but if you feel strongly about investigating, I can try to improve this code in our use case.

web/redux/create-async-migrate.js
27 ↗(On Diff #25832)

It might happen in our case, so let's discuss it in place where it will be introduced (D7668)

tomek requested changes to this revision.May 9 2023, 1:38 AM
In D7658#229577, @kamil wrote:

@tomek questions you added are valid but those are questions to the redux-persist author. I wanted to copy-paste code from source code to preserve old logic, as this returns function which will be used inside redux-persist logic. I only updated from the sync function which returns a promise - to implicitly return a promise from the async function (which will allow us to put await before any migration).

Any additional modification could cause some undefined behaviors, but if you feel strongly about investigating, I can try to improve this code in our use case.

Ah, that makes sense! But now it brings more serious issue: we shouldn't just copy and paste code from a library. Ideally, we would like to avoid modification and e.g. wrap code with our logic. If that's not possible, we can e.g. patch the library. If using this code directly is the only option, we should rewrite this code using the original one as an inspiration.

This revision now requires changes to proceed.May 9 2023, 1:38 AM
In D7658#230011, @tomek wrote:

Ah, that makes sense! But now it brings more serious issue: we shouldn't just copy and paste code from a library. Ideally, we would like to avoid modification and e.g. wrap code with our logic. If that's not possible, we can e.g. patch the library. If using this code directly is the only option, we should rewrite this code using the original one as an inspiration.

Due to complexity and some legacy problems patching the library was too hard. I deeply analyzed function from redux-persist and based on this knowledge (arguments, return type, when can throw, what should do) I provided my own implementation, using existing one only as an inspiration.

tomek added inline comments.
web/redux/create-async-migrate.js
27 ↗(On Diff #26354)

Do we use a similar approach in other parts of the codebase?

This revision is now accepted and ready to land.May 10 2023, 9:31 AM
web/redux/create-async-migrate.js
27 ↗(On Diff #26354)

Hmm, probably not, but the previous version of the function worked like this so I think It'll be best to keep it as it was