[web] Handle notifications in service worker
ClosedPublic
Actions

Authored by michal on Feb 22 2023, 2:39 AM.

Details

Reviewers

tomek
atul
kamil

Commits

rCOMMdfb3e013cfdd: [web] Handle notifications in service worker

Summary

This diff modifies the service worker to actually handle the push event and display a notification, additionaly it modifies the other events:

'install' -> adds a skipWaiting so it will immediately take over the previous service worker
'activate' -> clients.claim() so it will claim tabs that aren't controlled by a service worker (while this tab wouldn't have a event listener and wouldn't be able to navigate to a chat when user click a notification, we will still be able to focus on it)

event.waitUntil is needed because otherwise the browser could put the service worker to sleep before the promises are resolved.

Test Plan

Rest if the notification shows up after getting a push event

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Feb 22 2023, 2:39 AM

Herald added a subscriber: ashoat. · View Herald TranscriptFeb 22 2023, 2:39 AM

michal added inline comments.Feb 22 2023, 2:42 AM

web/push-notif/service-worker.js
3–8 ↗	(On Diff #22923)	Unfortunately, these types don't exist in flow so I had to type them myself based on MDN.
10–11 ↗	(On Diff #22923)	While these exist in flow, eslint complains that they aren't defined. Instead of this, we could add them in `.eslintrc` in `globals` but then they would be available in the whole `web` which isn't great.

michal added a parent revision: D6820: [web] Add notification permission modal.Feb 22 2023, 2:52 AM

michal added a child revision: D6833: [web] Allow users to enable push notifs.

Harbormaster completed remote builds in B16762: Diff 22923.Feb 22 2023, 2:54 AM

michal requested review of this revision.Feb 22 2023, 2:54 AM

tomek accepted this revision.Feb 28 2023, 3:04 AM

tomek added inline comments.

web/push-notif/service-worker.js
4 ↗	(On Diff #22923)	If this is specific to push notifications, maybe we can have a more concrete type (with body, prefix, etc.)? Otherwise we should keep `Object` here and cast a result of calling `json()` immediately in push event handler.
22 ↗	(On Diff #22923)	Can we cast `data` so that all the fields have types assigned?

This revision is now accepted and ready to land.Feb 28 2023, 3:04 AM

Type the json from the web push API. The PushMessageData is taken from the official web api types so I think it's better to keep them in sync and only later type them as our own type.

Harbormaster completed remote builds in B16992: Diff 23246.Feb 28 2023, 9:46 AM

Closed by commit rCOMMdfb3e013cfdd: [web] Handle notifications in service worker (authored by Michal Gniadek <michal.gniadek@swmansion.com>). · Explain WhyMar 1 2023, 12:51 AM

This revision was automatically updated to reflect the committed changes.

Michal Gniadek <michal.gniadek@swmansion.com> added a commit: rCOMMdfb3e013cfdd: [web] Handle notifications in service worker.