[lib] Fix SQLite migrations
ClosedPublic
Actions

Authored by tomek on Oct 2 2024, 5:04 AM.

Details

Reviewers

kamil
ashoat
atul

Commits

rCOMM85789c85773e: [lib] Fix SQLite migrations

Summary

The migrations are broken because we weren't using the right shape of the specification. This diff fixes it and introduces necessary type checks.

https://linear.app/comm/issue/ENG-9467/redux-migrations-are-broken

Test Plan

Tested this by creating a new migration on the web where a new draft is created. Run the app, and reopened it. Checked if the SET_CLIENT_DB_STORE action contains the draft.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tomek created this revision.Oct 2 2024, 5:04 AM

tomek added inline comments.Oct 2 2024, 5:08 AM

native/redux/persist.js
1371 ↗	(On Diff #44820)	Even adding this check doesn't force Flow to verify the migrations' types so we have to explicitly annotate each migration.

Fix the import

Harbormaster completed remote builds in B32012: Diff 44821.Oct 2 2024, 5:25 AM

tomek requested review of this revision.Oct 2 2024, 5:25 AM

tomek edited the test plan for this revision. (Show Details)Oct 2 2024, 5:25 AM

ashoat accepted this revision.Oct 2 2024, 5:31 AM

ashoat added inline comments.

lib/utils/migration-utils.js
207 ↗	(On Diff #44821)	I would name this `MigrationFunction`
210 ↗	(On Diff #44821)	Why remove `PersistedState` here?

This revision is now accepted and ready to land.Oct 2 2024, 5:31 AM

Harbormaster completed remote builds in B32011: Diff 44820.Oct 2 2024, 5:34 AM

tomek added inline comments.Oct 2 2024, 8:51 AM

lib/utils/migration-utils.js
210 ↗	(On Diff #44821)	`PersistedState` is an optional `AppState`. We were telling here that it can be used as an argument, but our migration functions were assuming that the `state` is mandatory. This issue was discovered by making the migration types explicit.