Remove ServiceWorker Page Control Requirement

[jkasten2]

Description

1 Line Summary

Remove ServiceWorker page control requirement.

Details

Summary

This is a quick 2 minute summary, recommend also seeing the full spike work here after reading this summary.
Removes the requirement of the service worker scope to be at or higher than the page you are viewing. (AKA the ServiceWorker has control of the page)

• You may have known you could already customize the path and scope of the OneSignal ServiceWorker before this but it still meant the user has to be on a page in it's scope.

APIs that assumed only one ServiceWorker is on a domain has been replaced with navigator.serviceWorker.getRegistration(scope).

• Examples; navigator.serviceWorker.controller & navigator.serviceWorker.ready

Dev Sandbox has been updated to a non-root scope to demonstrate these changes work.

What this means

• No need to merge ServiceWorker files per the "Integrating Multiple Service Workers" guide to support the following.
• Compatibility with AMP, when using your own domain (AKA not Google proxied)
• Compatibility with PWA
• And any other service worker that requires page control

What is PR does NOT do

• Does not change the default scope
  • This is intended so there is no breaking changes to those who have already setup this SDK. A change in the default will be done in the future.
• Does not allow more than one OneSignal ServiceWorker per domain. Example en/push/onesignal and es/push/onesignal
  • This is because the same index.db, sessionStorage, and others would be shared.

Full Details

See issue #615 for the full research done before this PR was created.

Systems Affected

• WebSDK
• Backend
• Dashboard

Validation

Tests

Tests have been updated and pass. Need to evaluate changes if we some new tests.

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
  • Tested Edge(Chromium), and Firefox on Windows 10.
  • Tested Chrome and Safari on macOS
  • Tested both HTTP and HTTPS sites
  • Tested ServiceWorker upgrading versions
  • Tested on_focus and on_session works
  • Tested notification open focuses an existing tab when it should
  • Tested with and without another SW at the root scope along with OneSignal being sub-scoped.
• I have included test coverage for these changes or explained why they are not needed
  • Added ServiceWorkerUtilHelper tests
  • Added throws to a new mock for service worker APIs that require page control which we should never use again.

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
  • Some new OneSignal.context were introduced into this PR as injection of the serviceWorkerManager would have required a number of changes. I recommend instead we come up with a strategy on removing this in another unrelated PR.

Browser behavior

Page to SW Message race condition

I had a concern about how soon you can send a message to a ServiceWorker after registering it. It seems right away as long as you await on navigator.serviceWorker.getRegistration() and check all 3 possible state the worker can be in.

Proof of this behavior:

Page:

async function testThis() {
  await navigator.serviceWorker.register("pwa-sw.js");
  var getReg = await navigator.serviceWorker.getRegistration();
  var curSw = getReg.installing || getReg.waiting || getReg.active;
  curSw.postMessage("[Page -> SW] Hello from page!");
  console.log("getReg.installing: " + getReg.installing + " waiting: " + getReg.waiting + " active: " + getReg.active);
}
testThis();

pwa-sw.js:

self.addEventListener('message', function(event) {
    console.log("[SW Context] Got Messsage: ", event);
});

Results:

02:26:22.019 getReg.installing: [object ServiceWorker] waiting: null active: null debugger eval code:9:18
02:26:22.019 [SW Context] Got Messsage:  [object ExtendableMessageEvent] pwa-sw.js:3:13
02:26:22.020 undefined

Confirmed this on Edge(Chromium) 88.0.705.63 , Firefox 85.0.2, and Safari 13.1.2.

Subscribe to push race condition

I had a concern about how soon you can subscribe for push after the service worker is registered. As long as you await on navigator.serviceWorker.getRegistration() this isn't an issue in my testing.
Page:

/**
 * urlBase64ToUint8Array
 * 
 * @param {string} base64String a public vavid key
 */
function urlBase64ToUint8Array(base64String) {
    var padding = '='.repeat((4 - base64String.length % 4) % 4);
    var base64 = (base64String + padding)
        .replace(/\-/g, '+')
        .replace(/_/g, '/');

    var rawData = window.atob(base64);
    var outputArray = new Uint8Array(rawData.length);

    for (var i = 0; i < rawData.length; ++i) {
        outputArray[i] = rawData.charCodeAt(i);
    }
    return outputArray;
}

async function testThis() {
  await navigator.serviceWorker.register("pwa-sw.js");
  var getReg = await navigator.serviceWorker.getRegistration();  
  var pushOptions = {
    userVisibleOnly: true,
    applicationServerKey: urlBase64ToUint8Array(
        'BEl62iUYgUivxIkv69yViEuiBIa-Ib9-SkvMeAtA3LFgDzkrxZJjSgSnfckjBJuBkr3qBUYIHBQFLXYp5Nksh8U'
      )
  };
  var pushSub = await getReg.pushManager.subscribe(pushOptions);
  console.log("pushSub: " + JSON.stringify(pushSub.toJSON()));
}
testThis();

Results:

00:00:35.533
/**
 * urlBase64ToUint8Array
 * 
 * @param {string} base64String a public vavid key
 */…
00:00:35.570
Promise { "pending" }
​
<state>: "fulfilled"
​
<value>: undefined

00:00:35.696 pushSub: {"endpoint":"https://updates.push.services.mozilla.com/wpush/v2/gAAAAABgL…wt29fu6xANwYULvF8u26_o5kNRL18yaTtkUjUkLZBDyguWSaJJWHsr95J87E","keys":{"auth":"HoHFMcrC6byl7BxPpXNF0A","p256dh":"BFT49CnizDrRFdzqQRHSmdRUjKDWvwKco6jzzmKOTFgfbSRjEuBokz0CZxgE6J0bwKnxSCapQKqIp7W7a3tJP0o"}}

Confirmed this on Edge(Chromium) 88.0.705.63 and Firefox 85.0.2.

Checklist

• I have included screenshots/recordings of the intended results or explained why they are not needed

---

Related Tickets

• Issue Investigate removing page controller requirement for the ServiceWorker  #615
• PR Fixes registration with non-root service workers #389

---

This change is 

[itrush]

Reviewed 21 of 21 files at r1, 1 of 1 files at r2.
Reviewable status: 15 of 23 files reviewed, 2 unresolved discussions (waiting on @itrush, @jkasten2, and @rgomezp)

---

src/helpers/SubscriptionHelper.ts, line 165 at r3 (raw file):

OneSignal.context.serviceWorkerManager

possibly replace with function argument to keep helper clean of dependencies

---

src/helpers/page/ServiceWorkerUtilHelper.ts, line 20 at r3 (raw file):

throw new Error("Could not find an available ServiceWorker instance!");

just be careful about this error, it may break people's sites

[itrush]

Reviewed 8 of 8 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jkasten2 and @rgomezp)

[jkasten2]

Reviewable status: 18 of 30 files reviewed, 2 unresolved discussions (waiting on @itrush and @rgomezp)

---

src/helpers/SubscriptionHelper.ts, line 165 at r3 (raw file):

Previously, itrush (Iryna Trush) wrote…

OneSignal.context.serviceWorkerManager

possibly replace with function argument to keep helper clean of dependencies

I move the access to OneSignal.context.serviceWorkerManager down one level to SubscriptionHelper.getRawPushSubscription to make things a bit more separated. However the call stack for this goes through MainHelper.createDeviceRecord which is also static and has 7 entry points. This helper layering isn't the best design and adding a 3 param to each seems like a side step.

This points to a larger issues of using state through statics instead of creating instance where we can do dependency injection.

---

src/helpers/page/ServiceWorkerUtilHelper.ts, line 20 at r3 (raw file):

Previously, itrush (Iryna Trush) wrote…

throw new Error("Could not find an available ServiceWorker instance!");

just be careful about this error, it may break people's sites

Fixed! Turned this into a warning. On the consuming end added a null check.

[itrush]

Reviewed 14 of 14 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jkasten2 and @rgomezp)

---

src/helpers/SubscriptionHelper.ts, line 165 at r3 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

I move the access to OneSignal.context.serviceWorkerManager down one level to SubscriptionHelper.getRawPushSubscription to make things a bit more separated. However the call stack for this goes through MainHelper.createDeviceRecord which is also static and has 7 entry points. This helper layering isn't the best design and adding a 3 param to each seems like a side step.

This points to a larger issues of using state through statics instead of creating instance where we can do dependency injection.

yeah, it's asking for a nice refactor :) but if it's too involved, let's keep it simple for now

---

src/managers/ServiceWorkerManager.ts, line 89 at r4 (raw file):

····

nit: extra spaces

---

test/support/mocks/service-workers/models/MockServiceWorkerContainer.ts, line 7 at r4 (raw file):

·

nit: trailing whitespace

---

test/support/mocks/service-workers/models/MockServiceWorkerContainerWithAPIBan.ts, line 13 at r4 (raw file):

··

nit: trailing spaces

---

test/support/mocks/service-workers/models/MockServiceWorkerContainerWithAPIBan.ts, line 46 at r4 (raw file):

}

nit: empty line at the end of the file

---

test/unit/helpers/page/ServiceWorkerUtilHelper.ts, line 34 at r4 (raw file):

});

// See test/unit/manager/ServiceWorkerManager.ts for other tests that cover this class

nit: new line at the end of the file

---

test/unit/managers/ServiceWorkerManager.ts, line 187 at r3 (raw file):

navigator.serviceWorker.controller

just to double-check: if someone writes it like this in tests, it would throw too?

---

test/unit/meta/mockPushManager.ts, line 57 at r4 (raw file):

subscribedSubscription

subscribed subscription, buttery butter 🙃

[jkasten2]

Reviewable status: 25 of 30 files reviewed, 6 unresolved discussions (waiting on @itrush and @rgomezp)

---

src/helpers/SubscriptionHelper.ts, line 165 at r3 (raw file):

Previously, itrush (Iryna Trush) wrote…

yeah, it's asking for a nice refactor :) but if it's too involved, let's keep it simple for now

Thanks!

---

src/managers/ServiceWorkerManager.ts, line 89 at r4 (raw file):

Previously, itrush (Iryna Trush) wrote…

····

nit: extra spaces

Fixed!

---

test/support/mocks/service-workers/models/MockServiceWorkerContainer.ts, line 7 at r4 (raw file):

Previously, itrush (Iryna Trush) wrote…

·

nit: trailing whitespace

Fixed!

---

test/support/mocks/service-workers/models/MockServiceWorkerContainerWithAPIBan.ts, line 13 at r4 (raw file):

Previously, itrush (Iryna Trush) wrote…

··

nit: trailing spaces

Fixed!

---

test/support/mocks/service-workers/models/MockServiceWorkerContainerWithAPIBan.ts, line 46 at r4 (raw file):

Previously, itrush (Iryna Trush) wrote…

}

nit: empty line at the end of the file

Fixed!

---

test/unit/helpers/page/ServiceWorkerUtilHelper.ts, line 34 at r4 (raw file):

Previously, itrush (Iryna Trush) wrote…

nit: new line at the end of the file

Fixed!

---

test/unit/managers/ServiceWorkerManager.ts, line 187 at r3 (raw file):

Previously, itrush (Iryna Trush) wrote…

navigator.serviceWorker.controller

just to double-check: if someone writes it like this in tests, it would throw too?

Yes! Also will throw if someone adds this to something in src /and it runs in any test.

---

test/unit/meta/mockPushManager.ts, line 57 at r4 (raw file):

Previously, itrush (Iryna Trush) wrote…

subscribedSubscription

subscribed subscription, buttery butter 🙃

haha, just making sure someone doesn't think it is I can't believe it's not butter! 😆

Anyway I picked a better name.

[jkasten2]

Subscribe to push race condition

I had a concern about how soon you can subscribe for push after the service worker is registered. As long as you await on navigator.serviceWorker.getRegistration() this isn't an issue in my testing.
Page:

/**
 * urlBase64ToUint8Array
 * 
 * @param {string} base64String a public vavid key
 */
function urlBase64ToUint8Array(base64String) {
    var padding = '='.repeat((4 - base64String.length % 4) % 4);
    var base64 = (base64String + padding)
        .replace(/\-/g, '+')
        .replace(/_/g, '/');

    var rawData = window.atob(base64);
    var outputArray = new Uint8Array(rawData.length);

    for (var i = 0; i < rawData.length; ++i) {
        outputArray[i] = rawData.charCodeAt(i);
    }
    return outputArray;
}

async function testThis() {
  await navigator.serviceWorker.register("pwa-sw.js");
  var getReg = await navigator.serviceWorker.getRegistration();  
  var pushOptions = {
    userVisibleOnly: true,
    applicationServerKey: urlBase64ToUint8Array(
        'BEl62iUYgUivxIkv69yViEuiBIa-Ib9-SkvMeAtA3LFgDzkrxZJjSgSnfckjBJuBkr3qBUYIHBQFLXYp5Nksh8U'
      )
  };
  var pushSub = await getReg.pushManager.subscribe(pushOptions);
  console.log("pushSub: " + JSON.stringify(pushSub.toJSON()));
}
testThis();

It turned out this wasn't always true. If another service worker is registered already under a scope of the newly registered one you may get the first one. Example first service worker scope could be / and the 2nd one /my-scope you may sometimes get the service worker at / and something the one at /my-scope.

This part of the logic from this PR was corrected in PR #1136