[lib] Update `MinimallyEncoded*Info` types to be `$ReadOnly`
ClosedPublic
Actions

Authored by atul on Nov 8 2023, 2:11 PM.

Details

Reviewers

ashoat
ginsu
tomek
rohan

Commits

rCOMM6e7b8508c797: [lib] Update `MinimallyEncoded*Info` types to be `$ReadOnly`

Summary

Updated the MinimallyEncoded*Info types to be $ReadOnly since spreading the "base" types drops the "read only" property and makes them writable... which caused some Flow issues.

Also going ahead and adding minimallyEncoded discriminator property to all of the types as we did for MinimallyEncodedRawThreadInfo in D9766 to help Flow refine types.

Test Plan

Flow, updated unit tests

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Nov 8 2023, 2:11 PM

Herald added a subscriber: will. · View Herald TranscriptNov 8 2023, 2:11 PM

Harbormaster completed remote builds in B23936: Diff 32979.Nov 8 2023, 5:13 PM

atul requested review of this revision.Nov 8 2023, 5:13 PM

tomek accepted this revision.Nov 9 2023, 3:39 AM

tomek added inline comments.

lib/permissions/minimally-encoded-thread-permissions.js
224 ↗	(On Diff #32979)	Not sure if it is likely that we will have some other encoding, but a more maintainable solution might be to have e.g. `+encoding: 'plain' \| 'minimal'` instead of a boolean flag. We can also keep the current approach and change it in the future if needed.
224–226 ↗	(On Diff #32979)	How about moving `minimallyEncoded` after spreading so that it won't get accidentally overridden?

This revision is now accepted and ready to land.Nov 9 2023, 3:39 AM

atul added inline comments.Nov 9 2023, 11:53 AM

lib/permissions/minimally-encoded-thread-permissions.js
224 ↗	(On Diff #32979)	Yeah figured boolean was simpler because we could do `if (roleInfo.minimallyEncoded)` instead of `if (roleInfo.minimallyEncoded === 'minimal)` for now. Hopefully we won't have yet another encoding any time soon, but will switch to enum if that comes up
224–226 ↗	(On Diff #32979)	Yeah seems reasonable, also might make the spread "less hidden"? Will update this diff