Details
Diff Detail
- Repository
- rCOMM Comm
- Branch
- arcpatch-D11680 (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
web/redux/persist.js | ||
---|---|---|
416–420 ↗ | (On Diff #39192) | Opted for this instead of something like so that this migration doesn't fail silently if message in SQLite is malformed. |
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? |
web/redux/persist.js | ||
---|---|---|
403 ↗ | (On Diff #39192) | I was following the pattern in migration 3: Without the invariant, we get the following flow errors: 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; } |
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. |