Page MenuHomePhabricator

[lib] Remove $FlowFixMes in message-reducer.js
ClosedPublic

Authored by ashoat on Dec 5 2023, 8:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jan 5, 7:00 AM
Unknown Object (File)
Wed, Dec 25, 8:04 PM
Unknown Object (File)
Wed, Dec 18, 9:17 PM
Unknown Object (File)
Tue, Dec 17, 3:24 AM
Unknown Object (File)
Tue, Dec 17, 3:24 AM
Unknown Object (File)
Tue, Dec 17, 3:24 AM
Unknown Object (File)
Tue, Dec 17, 3:24 AM
Unknown Object (File)
Tue, Dec 17, 3:23 AM
Subscribers

Details

Summary

This addresses ENG-4285. Will explain changes inline.

Test Plan

Flow

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lib/reducers/message-reducer.js
1298 ↗(On Diff #34323)

We can't simply remove these any-casts because Flow is right that there is an error here.

We cannot treat { ...singleMedia, ...mediaUpdate } as a Media, even after refining singleMedia to EncryptedImage and mediaUpdate to Partial<EncryptedImage>.

The reason is that EncryptedImage is a union type that sometimes has holder and sometimes has blobURI. If singleMedia has one and mediaUpdate has the other, we end up with both. But having both holder and blobURI is not a valid EncryptedImage.

The same issue exists for EncryptedVideo.

Ironically, the best solution here is to introduce branching similar to the branching I was able to remove in the later part of the file.

1299 ↗(On Diff #34323)

As an alternative to throwing away holder, we could add an invariant that mediaUpdate does not have one. I wasn't sure what was better

1383 ↗(On Diff #34323)

This was actually easy to remove!! Great to see that the new Flow is not as annoying about unnecessary branching.

ashoat requested review of this revision.Dec 5 2023, 8:56 PM
bartek added inline comments.
lib/reducers/message-reducer.js
1298 ↗(On Diff #34323)

The reason is that EncryptedImage is a union type that sometimes has holder and sometimes has blobURI. If singleMedia has one and mediaUpdate has the other, we end up with both. But having both holder and blobURI is not a valid EncryptedImage.

In practice such situation should never happen - there's either holder in both or blobURI in both. But looking at types only, that makes sense

1299 ↗(On Diff #34323)

Not sure either, failing with invariant seems like an overkill, however notifying the developer that this happened would be nice to have.
I'm slightly leaning towards your current approach.

This revision is now accepted and ready to land.Dec 6 2023, 1:22 AM
lib/reducers/message-reducer.js
1299 ↗(On Diff #34323)

however notifying the developer that this happened would be nice to have

I added a console.log

This revision was landed with ongoing or failed builds.Dec 6 2023, 7:14 AM
This revision was automatically updated to reflect the committed changes.