Page MenuHomePhabricator

[web] Register service worker for notifs
ClosedPublic

Authored by michal on Feb 21 2023, 9:12 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Feb 21, 12:49 PM
Unknown Object (File)
Wed, Feb 21, 12:49 PM
Unknown Object (File)
Wed, Feb 21, 12:49 PM
Unknown Object (File)
Wed, Feb 21, 12:48 PM
Unknown Object (File)
Wed, Feb 21, 12:48 PM
Unknown Object (File)
Sun, Feb 18, 2:42 AM
Unknown Object (File)
Sun, Feb 18, 12:28 AM
Unknown Object (File)
Sun, Feb 18, 12:17 AM
Subscribers

Details

Summary

We need to register a service worker to get push notifs from the browser. This diff registers the service worker from the endpoint exposed by the keyserver in one of the previous diffs on page load. For now this service worker only logs when installing and activating.

Test Plan
  • check if service worker is correctly registered (you can check that in the browser's dev tools)
  • check that the service worker is correctly updated after changing the service worker file

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 21 2023, 9:26 AM
Harbormaster failed remote builds in B16714: Diff 22868!
ashoat requested changes to this revision.Feb 22 2023, 11:16 AM

Confused why this needs to be a React component

web/push-notif/push-notifs-handler.js
7 ↗(On Diff #22920)

Is there a reason this needs to be in a React component? What if we just called this from the global scope?

16 ↗(On Diff #22920)

Just wondering... what does ESLint think is missing here? Based on my intuition about this ESLint rule, I would guess it thinks navigator and electron are both global/imported so don't need to be in the dep list

This revision now requires changes to proceed.Feb 22 2023, 11:16 AM

Moved eslint annotation to a later diff.

web/push-notif/push-notifs-handler.js
7 ↗(On Diff #22920)

While right now it's just using global objects, in the later diffs there are react hooks added inside this component.

16 ↗(On Diff #22920)

Sorry, I just forgot to put this in the correct diff when splitting this into multiple diffs. I'm going to move it the diffs that need this annotation.

This revision is now accepted and ready to land.Feb 24 2023, 3:13 PM