Update ServiceWorker if its href or scope changes #747

jkasten2 · 2021-02-19T13:38:14Z

Description

1 Line Summary

Update ServiceWorker if its href or scope changes. Allows a seamless transition for those who take advantage of the "Remove ServiceWorker Page Control Requirement" changes.

Details

Systems Affected

WebSDK
Backend
Dashboard

Validation

Tests

Tested the following scenario:

Subscribed for push with default OneSignal ServiceWorker settings.
- OneSignalSDKWorker.js on the root scope
Closed site
Changed scope by setting OneSignal.SERVICE_WORKER_PARAM to /push/onesignal/
Also someone may add their own service work in the same deploy
- Added a call to navigator.serviceWorker.register("pwa-sw.js"); to replace OneSignal at the root scope.
Opened site, observed both ServiceWorkers installed at different scopes.
Sent a push and observed it displayed correctly.

Tested on Edge and Firefox on Windows 10.

Info

Checklist

All the automated tests pass or I explained why that is not possible
I have personally tested this on my machine or explained why that is not possible
I have included test coverage for these changes or explained why they are not needed

Programming Checklist
Interfaces:

Don't use default export
New interfaces are in model files

Functions:

Don't use default export
All function signatures have return types
Helpers should not access any data but rather be given the data to operate on.

Typescript:

No Typescript warnings
Avoid silencing null/undefined warnings with the exclamation point

Other:

Iteration: refrain from using elem of array syntax. Prefer forEach or use map
Avoid using global OneSignal accessor for context if possible. Instead, we can pass it to function/constructor so that we don't call OneSignal.context

Info

Browser behavior

Question if two different SW files have `self.addEventListener('push'` which one gets the event?

Every ServiceWorkerRegistration instance has its own subscription endpoint. So the answer is simply which one you sent the push to..

Checklist

First time setup
Site was on root scope, changes to a non-root scope such as /push/onesignal/
Site has a root worked with both their SW importScripts and ours, want to migrate to OneSignal being under it own scope.
Ensure pushes display, and don't display twice.
I have included screenshots/recordings of the intended results or explained why they are not needed

Related Tickets

PR #745

This change is

* No code changes * Documenting the current state before I introduce a change in my next commit

* Moved logic into smaller parts that is used the generate the full href for the OneSignal ServiceWorker. - No result changes. * Simplied the else part where we check for ServiceWorkerActiveState - As we always want to install service worker A in all other cases. * The benifits of this refactor will be used in the next commit

jkasten2

Would be nice if there was more background about the problem this is trying to solve and ServiceWorker state it results. Such as keeping the existing worker incase it has more than just a OneSignal importScripts in it. And how getting calling getRegistration("/scope") will get higher scope if a specific one isn't present.

jkasten2 · 2021-02-23T22:15:11Z

src/managers/ServiceWorkerManager.ts


-      return permission === "granted";
+    // 5. We have a OneSignal ServiceWorker installed, but did the path or scope of the ServiceWorker change?
+    if (await this.changedServiceWorkerParams()) {


Future PR: ServiceWorkerActiveState.ThirdParty check isn't really needed if we are checking for the name already in this changedServiceWorkerParams method.

Future PR: ServiceWorkerActiveState.None could also be cleaned up, since this checks for presence. Consider if it is worth keeping enum however so it is easy to search for.

src/managers/ServiceWorkerManager.ts

itrush

Reviewed 1 of 1 files at r1, 5 of 6 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jkasten2 and @rgomezp)

src/managers/ServiceWorkerManager.ts, line 176 at r2 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Future PR: ServiceWorkerActiveState.None could also be cleaned up, since this checks for presence. Consider if it is worth keeping enum however so it is easy to search for.

👍

* Added a new changedServiceWorkerParams method which check if; - OneSignal ServiceWorker path, filename, or query params changed. - OneSignal ServierWorker scope changed * This is a new check that was added to shouldInstallWorker * A developer might change the OneSignal service worker params to take advantage of the PR #745 "Remove ServiceWorker Page Control Requirement" - This way the new OneSignal SW scope can be setup while their new root scoped SW can be installed at the same time.

* Multiple service workers are possible with browsers, if their scopes are different. Updated the mock to handle this.

* MockServiceWorkerContainer.getRegistration - Added match any SW's that are at a higher scope than the one we are querying for as they are under it's control. - This was observed behavoir when testing on real browsers.

jkasten2 requested review from itrush and rgomezp February 19, 2021 13:38

Base automatically changed from feat/remove_sw_page_control_requirement to master February 19, 2021 20:03

jkasten2 force-pushed the feat/update_sw_if_filename_or_scope_changes branch from 03713a3 to ab96860 Compare February 20, 2021 06:18

jkasten2 added 2 commits February 23, 2021 06:04

Added comments to shouldInstallWorker

615c719

* No code changes * Documenting the current state before I introduce a change in my next commit

jkasten2 force-pushed the feat/update_sw_if_filename_or_scope_changes branch from 24c2251 to 873a47a Compare February 23, 2021 14:04

jkasten2 commented Feb 23, 2021

View reviewed changes

itrush approved these changes Feb 24, 2021

View reviewed changes

jkasten2 added 5 commits February 23, 2021 21:18

Added more logging to the ServiceWorkerManager

dd39b2a

Multiple registartions MockServiceWorkerContainer

50dd84f

* Multiple service workers are possible with browsers, if their scopes are different. Updated the mock to handle this.

Scope fallback to MockServiceWorkerContainer

3bc851c

* MockServiceWorkerContainer.getRegistration - Added match any SW's that are at a higher scope than the one we are querying for as they are under it's control. - This was observed behavoir when testing on real browsers.

Added scope changed test

e1b89b1

jkasten2 force-pushed the feat/update_sw_if_filename_or_scope_changes branch from 575d45d to e1b89b1 Compare February 24, 2021 05:19

jkasten2 merged commit ac1f99a into master Feb 24, 2021

jkasten2 deleted the feat/update_sw_if_filename_or_scope_changes branch February 24, 2021 05:20

This was referenced Feb 24, 2021

151301 Release Commit #753

Merged

Investigate removing page controller requirement for the ServiceWorker #615

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ServiceWorker if its href or scope changes #747

Update ServiceWorker if its href or scope changes #747

jkasten2 commented Feb 19, 2021 •

edited

Loading

jkasten2 left a comment

jkasten2 Feb 23, 2021

jkasten2 Feb 23, 2021

itrush left a comment

Update ServiceWorker if its href or scope changes #747

Update ServiceWorker if its href or scope changes #747

Conversation

jkasten2 commented Feb 19, 2021 • edited Loading

Description

1 Line Summary

Details

Systems Affected

Validation

Tests

Info

Checklist

Info

Browser behavior

Question if two different SW files have self.addEventListener('push' which one gets the event?

Checklist

Related Tickets

jkasten2 left a comment

Choose a reason for hiding this comment

jkasten2 Feb 23, 2021

Choose a reason for hiding this comment

jkasten2 Feb 23, 2021

Choose a reason for hiding this comment

itrush left a comment

Choose a reason for hiding this comment

jkasten2 commented Feb 19, 2021 •

edited

Loading

Question if two different SW files have `self.addEventListener('push'` which one gets the event?