Page MenuHomePhabricator

[lib] Change type of `ThinRawThreadInfo.members` from `MemberInfoWithPermissions` to `MemberInfoSansPermissions`
ClosedPublic

Authored by atul on Jun 27 2024, 12:31 PM.
Tags
None
Referenced Files
F3345811: D12594.id42486.diff
Fri, Nov 22, 7:09 AM
Unknown Object (File)
Fri, Nov 15, 10:59 AM
Unknown Object (File)
Fri, Nov 15, 10:41 AM
Unknown Object (File)
Fri, Nov 15, 10:29 AM
Unknown Object (File)
Fri, Nov 15, 10:18 AM
Unknown Object (File)
Fri, Nov 15, 6:19 AM
Unknown Object (File)
Thu, Nov 14, 4:20 PM
Unknown Object (File)
Thu, Nov 14, 10:02 AM
Subscribers

Details

Summary

Start of stack replacing stack that started with D12469.

This diff makes the change to ThinRawThreadInfo.members which intentionally introduces flow issues which will be addressed in the rest of the stack.

Also snuck in stripping out MemberInfo.permissions from test data to reduce flow noise.

We're at 5 flow issues instead of the ~64 we had after D12469. Think this is largely due to stack ending in D12496, but will check each of the places with flow issues in previous stack to make sure nothing is broken quietly.


Depends on D12554

Test Plan

NA, flow.

This change ONLY changes flow types. We still need to update validators, persisted data, keyserver endpoint, client migrations, etc. which will be handled in subsequent diffs.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Some reminders to myself from abandoned stack that stack starting with this diff will replace:

https://phab.comm.dev/D12557#356476
https://phab.comm.dev/D12574#356485

  • will need to update validator AND make sure we have validators for previous/new ThinRawThreadInfo.members included in convertClientDBThreadInfoToRawThreadInfo to avoid breaking clients. This is crucial because convertClientDBThreadInfoToRawThreadInfo is run once upon client upgrade before migrations in persist.jsare run. This is what broke things with removing .isDefault migration.
Harbormaster returned this revision to the author for changes because remote builds failed.Jun 27 2024, 12:44 PM
Harbormaster failed remote builds in B29967: Diff 41766!
atul requested review of this revision.Jun 27 2024, 12:45 PM
atul added inline comments.
lib/types/minimally-encoded-thread-permissions-types.js
174 ↗(On Diff #41766)

Actual non-test data related change.

This diff just changes the types, but I'm curious about the actual data. Is there going to be a migration later to strip this data from the client, alongside the createPendingThread changes in D12597?

This revision is now accepted and ready to land.Jul 1 2024, 1:09 PM

rebase after resequence

rebase, look like there are no ESLint issues. There's failing validator that gets fixed in later diff... though maybe it makes sense to cherry-pick D12758 and land it to resolve failing validator test for rest of stack.

ashoat requested changes to this revision.Jul 17 2024, 1:02 PM

Hmm looks like a bunch of Flow errors

This revision now requires changes to proceed.Jul 17 2024, 1:02 PM
atul requested review of this revision.Jul 17 2024, 1:03 PM

Hmm looks like a bunch of Flow errors

This diff was specifically to induce the flow issues which get resolved by subsequent diffs in stack.

We're at 5 flow issues instead of the ~64 we had after D12469. Think this is largely due to stack ending in D12496, but will check each of the places with flow issues in previous stack to make sure nothing is broken quietly.

Ah okay, sorry I missed that

This revision is now accepted and ready to land.Jul 17 2024, 1:13 PM

rebase after landing validator fix.

This revision was landed with ongoing or failed builds.Jul 18 2024, 1:37 PM
This revision was automatically updated to reflect the committed changes.