Page MenuHomePhabricator

[web-db] migrate entire redux-persist storage to SQLite
ClosedPublic

Authored by kamil on Apr 27 2023, 7:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 8:50 PM
Unknown Object (File)
Sat, Nov 23, 10:11 AM
Unknown Object (File)
Sat, Nov 23, 9:56 AM
Unknown Object (File)
Sat, Nov 2, 1:01 AM
Unknown Object (File)
Sat, Nov 2, 1:01 AM
Unknown Object (File)
Sat, Nov 2, 1:01 AM
Unknown Object (File)
Sat, Nov 2, 1:01 AM
Unknown Object (File)
Sat, Nov 2, 12:55 AM
Subscribers

Details

Summary

This code will switch redux-persist storage to this based on encrypted SQLite.

However, to make a gentle switch we need to look to old storage and migrate the data if possible.

Depends on D7667

Test Plan
  1. Enable this code
  2. Check is persisted content is not missing but stored in SQLite
  3. Data from old storage should be removed.

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, 8:00 AM
tomek requested changes to this revision.Apr 28 2023, 4:52 AM
tomek added inline comments.
web/redux/create-async-migrate.js
28–46 ↗(On Diff #25854)

Is it a standard way of doing this? Wondering if checking this during each migration makes sense. Is it possible that we would want to move from one store to the other without any new migration introduced?

This revision now requires changes to proceed.Apr 28 2023, 4:52 AM
kamil requested review of this revision.May 8 2023, 4:49 AM
kamil added inline comments.
web/redux/create-async-migrate.js
28–46 ↗(On Diff #25854)

Is it a standard way of doing this?

I didn't find any suggestion of how to do it properly, there is one issue on GitHub and my solution is similar to the comments there (link).

Wondering if checking this during each migration makes sense

We not checking in each migration, we check once, one app start, and rehydrated state is empty.

Is it possible that we would want to move from one store to the other without any new migration introduced?

Yes, this code will migrate storage engines without any new migrations.

I can explain two alternative solutions but I don't think they're better:

  1. Allow migrating empty state

According to this comment: https://phab.comm.dev/D7658?id=25832#inline-50059. Run through migrations and if the undefined state was an argument, return undefined, and then introduce the next migration which will be responsible for taking a look into the old storage engine.

  1. Use persistStore callback

This callback is called after rehydration and migrations, we can look into the old store but it's tough to propagate data to a new one.

Let me know what do you think.

kamil planned changes to this revision.May 9 2023, 2:03 AM

I tried a different approach (getting old storage inside migration) but it will not be better. We need to have a store version before the migration process, and in that case, this version is persisted via old storage, so we need to retrieve it here.

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

Is it possible that something goes wrong (e.g. an exception is thrown) and we remove old state before saving it to a new place? Can we purge only after the successful migration to SQLite?

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

In this case, I don't consider this as an issue. This code is responsible only for reading persisted data into memory (and changing it if there are some migrations) and then with the next modifications data will be persisted in the new engine (that being said I don't think we should leave this data here, and we can purge this). Problems with saving data into new storage are separate things which mean something is wrong with the database and we should fetch new data for the user - in this case, it'll be reloaded.