Page MenuHomePhabricator

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

Authored by ashoat on Dec 5 2023, 8:38 PM.
Tags
None
Referenced Files
F3741538: D10200.id34342.diff
Thu, Jan 9, 12:05 PM
F3740825: D10200.id34323.diff
Thu, Jan 9, 11:43 AM
F3731711: D10200.diff
Thu, Jan 9, 1:01 AM
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
Subscribers

Details

Summary

This addresses ENG-4285. Will explain changes inline.

Test Plan

Flow

Diff Detail

Repository
rCOMM Comm
Branch
ashoat/postflow
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/reducers/message-reducer.js
1298

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

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

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

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

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

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.