[Flow202][keyserver][skip-ci] [16/x] Address type errors in message-fetchers.js
ClosedPublic
Actions

Authored by ashoat on Nov 13 2023, 3:20 PM.

Details

Reviewers

rohan
inka
michal

Commits

rCOMM2b5ea9bd7446: [Flow202][keyserver][skip-ci] [16/x] Address type errors in message-fetchers.js

Summary

The Flow update forced me to type MessageSQLResultRow here, which surfaced some issues. Separating this diff out since there are some non-Flow changes:

Added an invariant that should never trigger: collapseQuery results should all have collapse_key.
Added an if (!row.id) continue to parseMessageSQLResult. This is necessary because technically id is nullable – in fetchCollapsableNotifs, if a notif points to a message that is missing in the messages table, this can happen. In that case we will just skip that message when fetching the collapsable notifs.
Stripped some awaits that were in front of parseMessageSQLResult (which doesn't return a Promise).

NOTE: CI will fail on this diff. I considered the possibility of fixing Flow errors BEFORE upgrading Flow, but it wasn't possible... in some cases, the fixes to support the new version of Flow caused errors in the old version. I could have hidden these type errors with $FlowFixMe lines and then later revert those, but that seemed like too much busy work.

Depends on D9864

Test Plan

Confirm the Flow errors go away

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ashoat created this revision.Nov 13 2023, 3:20 PM

Herald added a subscriber: tomek. · View Herald TranscriptNov 13 2023, 3:20 PM

ashoat added a child revision: D9866: [Flow202][keyserver][skip-ci] [17/x] Fix type parameterization of accountUpdateInputValidator.Nov 13 2023, 3:21 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 13 2023, 3:32 PM

Harbormaster failed remote builds in B24082: Diff 33181!

ashoat added inline comments.Nov 14 2023, 6:11 AM

keyserver/src/fetchers/message-fetchers.js
161–162	After I typed `MessageSQLResultRow`, Flow started complaining for both of these. Technically the old code was safe... JS has quirky behavior with objects where it basically casts all `number` keys to `strings`, and the `collapse_key` is guaranteed by the query to always be set (`collapseQuery` specifically queries for results that have a `collapse_key` set to a specific value) (I could have created a different type for the result of `collapseQuery`, but didn't seem that important)
217–219	This is necessary because technically `id` is nullable – in `fetchCollapsableNotifs`, if a notif points to a message that is missing in the `messages` table, this can happen. In that case we will just skip that message when fetching the collapsable notifs. (If this DB corruption occurred previously, we would have thrown an exception... so this probably counts as Flow identifying a type error)
348	This `await` (and all the ones below) are not necessary because `parseMessageSQLResult` does not return a `Promise`. Flow didn't notice this since it's technically valid JS (`await` a non-`Promise` just returns it back), but there's no reason to suspend execution here, so I removed the `await` keywords

ashoat requested review of this revision.Nov 14 2023, 6:18 AM

michal accepted this revision.Nov 15 2023, 1:35 AM

This revision is now accepted and ready to land.Nov 15 2023, 1:35 AM

ashoat retitled this revision from [Flow202][keyserver] [16/x] Address type errors in message-fetchers.js to [Flow202][keyserver][skip-ci] [16/x] Address type errors in message-fetchers.js.Nov 19 2023, 5:08 PM

This revision was landed with ongoing or failed builds.Nov 27 2023, 3:27 PM

Closed by commit rCOMM2b5ea9bd7446: [Flow202][keyserver][skip-ci] [16/x] Address type errors in message-fetchers.js (authored by ashoat). · Explain Why

This revision was automatically updated to reflect the committed changes.

ashoat added a commit: rCOMM2b5ea9bd7446: [Flow202][keyserver][skip-ci] [16/x] Address type errors in message-fetchers.js.