Page MenuHomePhabricator

[lib] Introduce blobURI multimedia message field
ClosedPublic

Authored by bartek on Jun 29 2023, 6:37 AM.
Tags
None
Referenced Files
F3405802: D8370.diff
Tue, Dec 3, 11:00 PM
Unknown Object (File)
Sat, Nov 30, 3:11 AM
Unknown Object (File)
Sat, Nov 23, 2:25 PM
Unknown Object (File)
Sat, Nov 23, 12:45 PM
Unknown Object (File)
Sat, Nov 23, 12:45 PM
Unknown Object (File)
Sat, Nov 23, 7:09 AM
Unknown Object (File)
Mon, Nov 18, 4:58 AM
Unknown Object (File)
Sun, Nov 10, 12:33 PM
Subscribers

Details

Summary

First part of ENG-3966. Details in the Linear task.
Replaced the holder field for Encrypted{Image,Video} with union holder | blobURI and updated validators accordingly

Added @michal because I made changes to validators.

Test Plan

This stack is tested altogether, CI will fail at this point due to Flow breaking changes which will be solved in subsequent diffs.

Actual test plan in the last diff.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek edited the test plan for this revision. (Show Details)
bartek published this revision for review.Jun 29 2023, 8:14 AM
lib/types/media-types.js
689 ↗(On Diff #28255)

Is this by mistake? It does not match the type

lib/types/media-types.js
689 ↗(On Diff #28255)

Yes, good catch. Leftover after experiments

ashoat requested changes to this revision.Jul 2 2023, 6:39 PM
ashoat added inline comments.
lib/types/media-types.js
680 ↗(On Diff #28255)

Same here as below

688 ↗(On Diff #28255)

Not sure there's a point to wrapping this in tShape. You could just declare an object, and skip the .meta.props extraction below

697 ↗(On Diff #28255)

Not sure what this export is for... I can't find any usages outside this file

735 ↗(On Diff #28255)

I'm not sure you can spread a union type. I think this will lead to issues.

You should reverse the approach and define the shared parts first:

type EncryptedVideoShared = {
  +id: string,
  +encryptionKey: string,
  +type: 'encrypted_video',
  +dimensions: Dimensions,
  +loop?: boolean,
  +thumbnailID: string,
  +thumbnailEncryptionKey: string,
  +thumbnailThumbHash: ?string,
};

export type EncryptedVideo =
  | { ...EncryptedVideoShared, +holder: string, +thumbnailHolder: string }
  | { ...EncryptedVideoShared, +blobURI: string, +thumbnailBlobURI: string };
This revision now requires changes to proceed.Jul 2 2023, 6:39 PM
lib/types/media-types.js
688 ↗(On Diff #28255)

Omitting this wasn't working in my initial solution but looks like it's working now, I'll do it

697 ↗(On Diff #28255)

It was here before my changes. I suspect @michal might know more

735 ↗(On Diff #28255)

Makes sense - this will keep the types in sync with validators too

lib/types/media-types.js
688

One advantage of wrapping it in tShape<T>({...}) is that flow will throw an error if the field names don't match between the validator and the type.

697 ↗(On Diff #28255)

When I was creating the validators, I exported them if the type they were based on was also exported.

ashoat added inline comments.
lib/types/media-types.js
688

Won't that happen on line 697 and 698 still?

This revision is now accepted and ready to land.Jul 3 2023, 10:39 AM
lib/types/media-types.js
688

Unfortunately, the flow types for tcomb are not that smart. The field check only works if you explicitly specify the generic T in tShape<T>.

This revision was landed with ongoing or failed builds.Jul 11 2023, 2:58 AM
This revision was automatically updated to reflect the committed changes.