Part of threadInfoFromRawThreadInfo(...).
Required changes to createPendingThread and sendPushNotifs.
Depends on D10715
Details
Details
flow, will make sure push notifs continue to work as expected.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- arcpatch-D10718 (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| keyserver/src/push/send.js | ||
|---|---|---|
| 139–143 ↗ | (On Diff #35839) | By passing { minimallyEncodePermissions: true } to rawThreadInfoFromServerThreadInfo we're "guaranteed" to get minimally encoded RawThreadInfos back. If we don't, that would indicate something has gone very wrong... so I think it's acceptable to use an invariant here. We could explore parameterizing rawThreadInfoFromServerThreadInfo based on options.minimallyEncodePermissions, but I'm not sure how to do that. CC @ashoat |
| lib/shared/thread-utils.js | ||
| 366 ↗ | (On Diff #35839) | Wow... glad I caught that role wasn't minimallyEncoded in the first revision of this diff (I guess first diff of this revision). flow didn't report any issues and we would've had malformed RawThreadInfos. Will take a sec, but going to write/generate some unit tests for this function. |
Comment Actions
Passing back for those unit tests and a question inline
| keyserver/src/push/send.js | ||
|---|---|---|
| 139–143 ↗ | (On Diff #35839) | Is there a reason we have to use such a peculiar API that is difficult to type? As an alternative, what if we just had two functions, one that returned a minimally-encoded type and one that didn't? |
| lib/shared/thread-utils.js | ||
| 366 ↗ | (On Diff #35839) | Would be good to investigate how Flow missed this. Unit tests sound like a good idea too |
| keyserver/src/push/send.js | ||
|---|---|---|
| 139–143 ↗ | (On Diff #35839) |
It's because rawThreadInfoFromServerThreadInfo accepts RawThreadInfoOptions:  which are passed in based on version checks at rawThreadInfoFromServerThreadInfo callsite:  I think it makes sense to maintain the symmetry w/ the other options that are passed in via codeVersion. Would the alternative be adding a separate argument to the function to toggle between encodings?
Would we want to duplicate functionality between the two, or would we want one function to be a wrapper around the other that either encodes/decodes the thread infos? FWIW will be updating rawThreadInfoFromServerThreadInfo as part of future "encoded permissions in memberships table" project, so it might make sense to sequence then? |
| lib/shared/thread-utils.js | ||
| 366 ↗ | (On Diff #35839) | Added unit test in D10818. Will create a task to document the flow issue and jot down any notes from investigating. |
| lib/shared/thread-utils.js | ||
|---|---|---|
| 366 ↗ | (On Diff #35839) | Documentation of the flow issue: https://linear.app/comm/issue/ENG-6596/investigate-why-flow-didnt-detect-issue-with-creatependingthread |