This differential enables encrypted notifications to have either keyserverID or senderDeviceID field. Theoretically we could have only senderDeviceID but this has disadvantages. Firstly we would need to handle old clients. Secondly we will need keyserverID anyway since thin threads have different mechanism to calculate unreadCount so we need a way to distuinguish whether a notif comes from keyserver or peer device.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/types/notif-types.js | ||
|---|---|---|
| 103 | It looks strange but I hand to unwind AndroidVisualNotificationPayloadBase here to make AndroidVisualNotification work. For some reason without unwinding flow would complain that AndroidVisualNotification doesn't have fields that it clearly has. | |
| web/push-notif/notif-crypto-utils.js | ||
| 68 | Soon we will modify this to enforce either senderDeviceID or keyserverID. Not just keyserverID. | |
| 114 | For some reason if I used keyserver directly flow would think that it is void | string even with the invariant above. | |
| keyserver/src/push/crypto.js | ||
|---|---|---|
| 283–293 | Is there a reason for keeping these ifs separated? | |
| lib/types/notif-types.js | ||
| 29 | It's quite surprising to see that a both keyserverID and senderDeviceID are valid values for SenderDeviceID - it feels like senderDeviceID should cover all the options because its name is too similar to the type name. Also, it's strange that SenderDeviceID is an object instead of a string. How about renaming the type name to e.g. SenderDeviceDescriptor? | |
| lib/types/notif-types.js | ||
|---|---|---|
| 103 | I don't think this structure makes sense anymore. If you're just adding a single +id?: string, to all of them, why not inline it? Perhaps if you inline +id?: string,, can you re-integrate AndroidVisualNotificationPayloadBase? And finally, does adding $ReadOnly wrappers fix the "unwinding" issue? Remember that read-only status is wiped when spreading a type, so it has to be re-applied at the new type, eg.: type SpreadType = { +something: string }; type OtherType = $ReadOnly<{ ...SpreadType, +blah: string, }> If you don't include $ReadOnly there, then something will not be read-only. | |
| lib/types/notif-types.js | ||
|---|---|---|
| 103 | We discussed this in our 1:1 and I was not able to reproduce the issues @marcin had which forced him to the take the "unwinding" approach here. We can return this to using a type spread of AndroidVisualNotificationPayloadBase: https://gist.github.com/Ashoat/bcb8778735d721eecea0f0ffbdfd2447 | |