Page MenuHomePhabricator

[web] Redux migration to clear `UNSUPPORTED<UPDATE_RELATIONSHIP>` messages
ClosedPublic

Authored by atul on Wed, Apr 17, 11:12 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 27, 3:13 AM
Unknown Object (File)
Tue, Apr 23, 5:23 PM
Unknown Object (File)
Tue, Apr 23, 1:15 PM
Unknown Object (File)
Mon, Apr 22, 3:34 PM
Unknown Object (File)
Sun, Apr 21, 3:02 PM
Unknown Object (File)
Sun, Apr 21, 9:25 AM
Unknown Object (File)
Sat, Apr 20, 1:45 PM
F1574322: 3f0434.png
Thu, Apr 18, 1:37 PM
Subscribers
None

Details

Summary

Effectively D11669, but for web instead of native.


Depends on D11669

Test Plan

Before migration (note messageID=84025):

image.png (884×1 px, 135 KB)

After migration (note absence of messageID=84025:

image.png (1×1 px, 153 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul created this revision.

modify invariant message

atul published this revision for review.Wed, Apr 17, 11:16 AM
atul added inline comments.
web/redux/persist.js
416–420 ↗(On Diff #39192)

Opted for this instead of something like

6e04a6.png (184×1 px, 20 KB)

so that this migration doesn't fail silently if message in SQLite is malformed.

atul edited the test plan for this revision. (Show Details)
ashoat requested changes to this revision.Wed, Apr 17, 3:24 PM

Worried about invariants

web/redux/persist.js
391 ↗(On Diff #39192)

Typo: early-exit

403 ↗(On Diff #39192)

Why is this invariant necessary? What makes it so that stores.store should exist here?

416–420 ↗(On Diff #39192)

Do we really want to crash the migration? What happens if the migration "fails silently"? What's the input on the user experience?

This revision now requires changes to proceed.Wed, Apr 17, 3:24 PM
web/redux/persist.js
403 ↗(On Diff #39192)

I was following the pattern in migration 3:

fbacb2.png (212×1 px, 58 KB)

Without the invariant, we get the following flow errors:

3f0434.png (1×2 px, 295 KB)

That said, the following would probably be a cleaner approach and avoid use of invariant:

// 2. Get existing `stores` from SQLite.
const stores = await sharedWorker.schedule({
  type: workerRequestMessageTypes.GET_CLIENT_STORE,
});

const messages: ?$ReadOnlyArray<ClientDBMessageInfo> =
  stores?.store?.messages;

if (messages === null || messages === undefined || messages.length === 0) {
  return state;
}

get rid of first invariant

atul marked an inline comment as done.

fix typo + remove second invaraint

web/redux/persist.js
416–420 ↗(On Diff #39192)

If message.content doesn't exist for a UPDATE_RELATIONSHIP message it won't be removed from localDB, but this situation should not be possible.

Updated to go with the previous check instead to avoid possibility of crashing mid-migration.

This revision is now accepted and ready to land.Fri, Apr 19, 6:42 AM
This revision was landed with ongoing or failed builds.Fri, Apr 19, 11:41 AM
This revision was automatically updated to reflect the committed changes.