This diff actually sends a web push notification using the web-push library. The library is initialized during configuration loading (added in one of the previous diffs). Then we can just send a notification with the device_token (which contains e.g. an endpoint and keys but it's all handled by web-push). Otherwise the logic should be similar to iOS and Android notifications.
Details
Details
Tested with next diff initializes notif sending on the web app side:
- test that notifs are send with correct values
- check that the results are saved inside of the notifications table
- check that errors are saved inside of the notifications table
- check that device_token is removed after unsuccessful push notif
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| keyserver/src/push/send.js | ||
|---|---|---|
| 621 ↗ | (On Diff #22827) | Can we type this more precisely? Object is basically any. We use it for iOS / Android because we have some untyped libraries there I think (could be wrong), but we should avoid introducing it here |
| 631 ↗ | (On Diff #22827) | Why do we need this id? I thought it was an iOS-specific thing |
| 746–749 ↗ | (On Diff #22827) | All new types should always be $ReadOnly. Please keep this in mind, and make sure it's addressed before putting diffs up in the future |
| 749 ↗ | (On Diff #22827) | What is the meaning of codeVersion for web? Is this just something for the future after ENG-961 is complete? |
| 753–755 ↗ | (On Diff #22827) | All new types should always be $ReadOnly. Please keep this in mind, and make sure it's addressed before putting diffs up in the future |
| 758 ↗ | (On Diff #22827) | Can we avoid Object? |
| keyserver/src/push/utils.js | ||
| 27 ↗ | (On Diff #22827) | We put the error code for APN up here. Should we put the error code for web in the same place? |
| 212 ↗ | (On Diff #22827) | Can we avoid Object? |
| 218–233 ↗ | (On Diff #22827) | I think this would be cleaner using .map |
Comment Actions
Fix types, move errors codes, answers to questions in inline comments.
| keyserver/src/push/send.js | ||
|---|---|---|
| 631 ↗ | (On Diff #22827) | There's no automatically generated ID for a notif by web push api, so I think it's good to have some id just to keep track of them. But they aren't necessary so I can remove them. |
| 749 ↗ | (On Diff #22827) | Yeah, I just wanted it to be consistent with the rest of the notif system for now. Currently, it will be -1 as set by the getDevicesByPlatform function. |
Comment Actions
Accepting to unblock, but would be great if we could avoid Object for new code. Not sure how easy it is to avoid here and whether the type there is predictable from whatever API we are getting it from
| keyserver/src/push/send.js | ||
|---|---|---|
| 751 ↗ | (On Diff #23059) | Can we avoid Object? |
| keyserver/src/push/utils.js | ||
| 204 ↗ | (On Diff #23059) | The style in the codebase is generally to make export type declarations inline with where the type is defined |
| 214 ↗ | (On Diff #23059) | Can we avoid Object? |
| 271 ↗ | (On Diff #23059) | Instead of here at the bottom |
Comment Actions
Move WebNotification type to lib so we can use it to type the push message in the service worker.