This differential implements our own types for APNs notifications as well as notif construction and encryption.
Details
Details
In order to test this code we need to hack the keyserver since in order to send APNs notif we need apn.Notification object anyway (it is required by node-apn lib). The steps are:
- Apply this patch: https://gist.github.com/marcinwasowicz/1af0a2958830fd9eb080e639779b1628. It makes the keyserver use our own types and new code to build the notification but before sending it creates custom apn.Notification object and overwrites its headers and compile methods to return data from our own notification.
- Test that APNs notifications, rescind and badge updates work for both iOS and macos
- Flow (unrelatedly).
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-8282
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/types/notif-types.js | ||
|---|---|---|
| 235–254 | I'm skeptical of nested type spreads like this | |
| lib/push/apns-notif-creators.js | ||
|---|---|---|
| 1 ↗ | (On Diff #41558) | Likely the only file that needs review here. |
| 46 ↗ | (On Diff #41558) | The logic is copy pasted from the keyserver but using our custom types. All assignments and conditions match the keyserver logic. The only difference is that node-apn library hides from us what goes into headers what into aps and what into custom payload. In this file this is explicit. |
Comment Actions
- Could you address @ashoat comments about spreads?
- For Tunnelbroker, I will need 3 things: deviceID, headers because those are specified when building HTTP2 request to APNs, and payload - stringified notif, built on the client and only passed to APNs by Tunnelbroker without modification, wondering if we shouldn't design types to avoid spreading headers along with other notif props, but you can also do it before calling Tunnelbroker, this is up to you.
| lib/push/crypto.js | ||
|---|---|---|
| 93 ↗ | (On Diff #41563) | shouldn't this be SenderDeviceDescriptor? |
| 114 ↗ | (On Diff #41563) | I think this is now APNsVisualNotification object |
| lib/types/notif-types.js | ||
| 202 ↗ | (On Diff #41563) | that's not all types, see here. Do we want to add types to only what we support? |
Comment Actions
Regarding my comments about nested spreads – I still think they're ugly, but we looked into this with @marcin in a 1:1 and I think Flow is now able to support them better than I remembered previously
| lib/types/notif-types.js | ||
|---|---|---|
| 202 ↗ | (On Diff #41563) | I deliberately narrowed the type only for the headers that we currently use. |