Details

Reviewers

tomek
marcin
• abosh
rohan
ginsu

Commits

rCOMMb2123ced3641: [lib] Introduce `translateClientDBMediaInfosToMedia(...)`

Summary

Given ClientDBMessageInfo we want to reconstruct an array of Media.

We'll consume translateClientDBMediaInfosToMedia(...) in multimedia-message-spec:rawMessageInfoFromClientDB to "unpersist" MULTIMEDIA messages stored in the SQLite messageStore.

Test Plan

Will be tested implicitly by subsequent diffs. Also included a simple unit test (will add some more later)

Diff Detail

Repository

rCOMM Comm

Branch

arcpatch-D5313_1 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Oct 6 2022, 11:02 AM

Herald added a subscriber: ashoat. · View Herald TranscriptOct 6 2022, 11:02 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 6 2022, 11:03 AM

Harbormaster failed remote builds in B12654: Diff 17388!

remove dummy variable i included for debugging

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 6 2022, 11:06 AM

Harbormaster failed remote builds in B12655: Diff 17389!

CI failing due to intermittent network issues on MariaDB's end

cut level of indentation

Harbormaster completed remote builds in B12656: Diff 17390.Oct 6 2022, 11:22 AM

atul added reviewers: rohan, ginsu.Oct 7 2022, 1:13 PM

atul added 1 blocking reviewer(s): tomek.Oct 10 2022, 6:52 PM

tomek requested changes to this revision.Oct 11 2022, 3:51 AM

tomek added inline comments.

lib/utils/message-ops-utils.js
78–92 ↗	(On Diff #17390)	I don't think this is a good idea to have so many invariants in this code. Usually an invariant should be used only when there's no other way to let Flow know the type or to express things that, when violated, are a programmer error. In this case it is completely valid for `clientDBMessageInfo` to have these fields empty, so a safer semantics for this method is to return en empty array for a message without media. In the past we had a couple of issues caused by extensive use of invariants and I think we should have good reasons to use them.
100–108 ↗	(On Diff #17390)	Can we use `translateClientDBMediaInfoToImage` here?

This revision now requires changes to proceed.Oct 11 2022, 3:51 AM

atul added inline comments.Oct 12 2022, 5:11 PM

lib/utils/message-ops-utils.js
78–92 ↗	(On Diff #17390)	Will update the function to return an empty array when there's no `media_infos` or `content`
100–108 ↗	(On Diff #17390)	It'll be removed altogether in a subsequent diff

address feedback

Harbormaster completed remote builds in B12766: Diff 17535.Oct 12 2022, 5:24 PM

tomek accepted this revision.Oct 13 2022, 8:23 AM

tomek added inline comments.

lib/utils/message-ops-utils.js
87–89 ↗	(On Diff #17535)	Not sure but maybe we can move it to the beginning of this function so that images and media have the same behavior

This revision is now accepted and ready to land.Oct 13 2022, 8:23 AM

atul added inline comments.Oct 13 2022, 8:27 AM

lib/utils/message-ops-utils.js
87–89 ↗	(On Diff #17535)	Ah yeah I tried doing that but `flow` wasn't happy if this wasn't right above const mediaInfos: $ReadOnlyArray<ClientDBMediaInfo> = clientDBMessageInfo.media_infos; But, I should remove the `invariant` in the `messageTypes.IMAGES` case and return empty array there as well for consistency.