-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Investigate removing page controller requirement for the ServiceWorker #615
Comments
In an AMP context, the eliminating of the controller requirement will allow for For example: <!-- Existing service worker controlling the page -->
<amp-install-serviceworker
src="https://example.com/serviceworker.js"
data-iframe-src="https://example.com/install-serviceworker.html"
data-scope="https://example.com/"
layout="nodisplay"
>
</amp-install-serviceworker>
<!-- New service worker for OneSignal scoped to a designated subdirectory to not conflict with existing controlling service worker -->
<amp-web-push
layout="nodisplay"
helper-iframe-url="https://example.com/amp-web-push-files/helper-iframe.html"
permission-dialog-url="https://example.com/amp-web-push-files/permission-dialog.html"
service-worker-url="https://example.com/amp-web-push-files/service-worker.js"
service-worker-scope="https://example.com/amp-web-push-files/"
></amp-web-push> |
@westonruter I believe your AMP example will work today. I'll try to confirm when I can. The reason why is that the OneSignal SDK isn't running on page itself when using AMP, but in an iframe. So as long as the path for The issue noted here is for non-AMP HTTPS sites with OneSignal being included on the page. It would be possible for someone to do basically the same implementation as |
FWIW, the "fake" scope that won't result in the SW controlling any pages approach is what the Firebase Messaging SDK uses. The main drawback is that the service worker update check will happen less frequently—pretty much only when there's a |
What's the status on this investigation? I'm getting more reports from users wanting to use OneSignal with the WordPress PWA plugin, but the page controller requirement is blocking them. |
@westonruter @jeffposnick Thanks for your help so far on this issue. I think we have a good chunk of the work done in a WIP PR but I have specific ServiceWorker question I would like your thoughts on. What you think of this implementation of getting an available navigator.serviceWorker
.register("/subdirectory/scoped-sw.js", { scope: "/subdirectory/" })
.then(reg => {
// ServiceWorker will be in 1 of 3 states; The postMessage payload will still arrive at the SW even if it isn't active
const availableWorker = reg.installing || reg.waiting || reg.active;
console.log(
"[index.html] Greetings to subdirectory/scoped-sw.js, are you there?"
);
availableWorker.postMessage("greet_from_index");
}) Working example: https://glitch.com/edit/#!/repeated-chiseled-air?path=index.html In comparison this is what you recommend @westonruter with your proof of concept a while back. avigator.serviceWorker
.register("/subdirectory/scoped-sw.js", { scope: "/subdirectory/" })
.then(reg => {
const onActive = () => {
console.log(
"[index.html] Greetings to subdirectory/scoped-sw.js, are you there?"
);
reg.active.postMessage("greet_from_index");
};
if (reg.active) {
onActive();
} else {
reg.addEventListener("updatefound", updateFoundEvent => {
const newWorker = reg.installing;
const onStateChange = stateChangeEvent => {
if ("activated" === newWorker.state) {
newWorker.removeEventListener("statechange", onStateChange);
onActive();
}
};
newWorker.addEventListener("statechange", onStateChange);
});
}
}) @westonruter @jeffposnick More details on our progress if you are interested in checking it out. I have updated the original issue in with a ton more detail and have created PR #745 which implements the changes. No need to review, just an FYI. |
The only thing to decide on is the order that you use in the I don't know how important it is to your code to always post a message to, e.g., the newly registered service worker vs. the previous service worker, which is what the ordering you're currently using would accomplish. Apologies if this was already obvious, but if for some reason you need to prioritize posting a message to the already-active service worker instead of the newest one, you'd want to list |
Status Update✅ Whats live and ready now@jeffposnick @westonruter What was shipped detailsThe update is automatic as almost everyone using the cdn URL: https://cdn.onesignal.com/sdks/OneSignalSDK.js
🚧Migration to different ServiceWorker Href@jeffposnick @westonruter Would be interested on your thought your approach here.
🚧Defaulting a Non-root scope for new setupsAs noted above, developers can choose to customize but this isn't automatic. I don't see any reason why we can't change the defaults for new OneSignal apps being setup but it will take some time to implement and document everywhere. 💡 Automatic migration to a non-default scope if we detect the SW contains only OneSignal codeThis is an idea I had there might be a way to migrate everyone off the root scope automatically when a browser comes back to the site. This would be fine if the OneSignal configured ServiceWorker file hosted on the site's domain was 100% guaranteed to only contain OneSignal service worker code. importScripts('https://cdn.onesignal.com/sdks/OneSignalSDKWorker.js'); This way we can ensure there is zero possibility of any non-OneSignal code that could cause a breaking change. |
I'm not 100% sure on the mechanisms for developers to upgrade, and what the "before" and "after" service workers generally look like, so I can't fully address your |
@jeffposnick Thanks, that make sense from a high level. I guess I am looking for a "I wish I knew X before I changed my ServiceWorker Scope and URL" kind of answer. One straightforward thing in a SW URL migration scenario is to keep hosting the "old" ServiceWorker URL if you don't want to avoid 404s from your backend each time you send a push. I have read through the following which I think has given me a deep enough understand to provide a solid path forward on this SW migration. But let me know if there is anything else I should read that would be helpful.
Also if you have any other good SW docs in general send them my way too. |
That's interesting—I've never had to use https://medium.com/dev-channel/two-http-headers-related-to-service-workers-you-never-may-have-heard-of-c8862f76cc60 is a good rundown of both of those headers. |
@jeffposnick Ah interesting, didn't know about the browser putting |
@jkasten2 What's the status on this issue? Is it still blocking compatibility between OneSignal and the Wordpress PWA plugin? |
@maxhartshorn We just released OneSignal-WordPress-Plugin 2.2.0 which uses a non-root ServiceWorker scope for new installs of the plugin. After there is a enough installs without any issues we will make it the default for those who upgrade too. For those using those the standard Javascript setup see the How Do I Integrate OneSignal's Service Worker Files? guide on how to add the OneSignal ServiceWorker to your site on a scope that won't conflict with other ServiceWorkers. |
@jkasten2 Excellent! So that means AMP compatibility should now be doable by serving an |
@westonruter Since there won't be a service worker scope to avoid then I believe determining if you include Could you point me to a setup guide and / or plugin code that this question is coming from so I can give a solid answer? |
I'm no expert on the <amp-web-push
layout="nodisplay"
helper-iframe-url="https://example.com/helper-iframe.html"
permission-dialog-url="https://example.com/permission-dialog.html"
service-worker-url="https://example.com/service-worker.js"
></amp-web-push> And then there are two custom elements for subscribing and unsubscribing to the notifications: <!-- A subscription widget -->
<amp-web-push-widget
visibility="unsubscribed"
layout="fixed"
width="250"
height="80"
>
<button on="tap:amp-web-push.subscribe">Subscribe to Notifications</button>
</amp-web-push-widget>
<!-- An unsubscription widget -->
<amp-web-push-widget
visibility="subscribed"
layout="fixed"
width="250"
height="80"
>
<button on="tap:amp-web-push.unsubscribe">
Unsubscribe from Notifications
</button>
</amp-web-push-widget> So for configuration with OneSignal, I think it's mainly a matter of just supplying the relevant URLs to
So when See also OneSignal/OneSignal-WordPress-Plugin#258. |
@westonruter Thanks for refresh on details here! This is great detail so I created a new issues to discuss in more details since this is getting off topic of the OP for the scope of the service worker. |
Current State
Update 2021-02-24:
See latest state in this comment: #615 (comment)
Old 2021-02-18
Today OneSignal's setup guide recommends uploading the required service worker .js file to the root of your site. This by default, sets the service worker scope to the location of the file.
Scope
Investigate what code in this repo depends on the page being controlled by the service worker. Next we need to document if there is an alternative API or alternative strategy to keep the functionality working.
Motivation
Browsers only allow one service worker to be installed per scope. This means if there is another service worker that needs to be on the root scope it will be replaced. OneSignal has a "Integrating Multiple Service Workers" work around for those who already have a service worker file they need to keep. In summary it involves a include only
importScripts
for all server works they need.ServiceWorker Page Control Dependencies
1.
navigator.serviceWorker.controller
Status - Confirmed alternative, needs implementation changes in SDK
For Page->ServiceWorker messages the following can be used.
Or you can get it based on scope -
navigator.serviceWorker.getRegistration(scope);
Thanks to @westonruter for these details and his proof of concept
Example of Page->ServiceWorker and ServiceWorker->Page
Thanks @jeffposnick for this complete demo:
https://noiseless-special-titanium.glitch.me/
2.
navigator.serviceWorker.ready
Similar to
navigator.serviceWorker.controller
we can't usenavigator.serviceWorker.ready
so these lines need to be updated.SubscriptionManager.ts:386
utils.ts:306
3.
clients.matchAll({ type: 'window', includeUncontrolled: false })
Status - Tested with Chrome, works as intended -
includeUncontrolled
One spot where we use
includeUncontrolled: false
instead oftrue
, looks safe to make thistrue
but need do additional testing. Specifically need to ensureon_focus
andon_session
are called when they should be.4. WorkerMessenger.isWorkerControllingPage
In following this chain I wasn't able to find an API that won't work. The method name should be updated however since it not actually checking if the page is controlled or not.
navigator.serviceWorker.getRegistration(location.href)
5.
navigator.serviceWorker.addEventListener('controllerchange', ...)
controllerchange
will never fire for subpage never visited by the user so we can't use this any more.waitUntilWorkerControlsPage
uses this6.
WindowClient.focus()
Status - Confirmed working in Chrome
Web docs do not bring up if the page has to be controlled or not by the service worker.
7. Reading
registration.scope
We are reading
registration.scope
in two spots which means this value is going to check when we make a non-root SW scope.self.location.origin;
works in a SW and is what we want.self.location.origin;
- This would keep the existing behavior if the SW scope was root before.8.
skipWaiting
&claim()
self.clients
- Is going to be empty as no one would be viewing a page on the/push/onesignal
path.self.skipWaiting()
- Again someone isn't going to have a page open to the scope but it could effect whatnavigator.serviceWorker.getRegistration("/push/onesignal/")
gives us.More details on Google's ServiceWorker lifecycle docs.
9.
ServiceWorkerRegistration.active
If we are no longer waiting for
controllerchange
event so we can't depend onServiceWorkerRegistration.active
only as it might benull
. We might no longer need to wait as we don't need to control of the page so we could most likely remove the.active
checks or get whichever state it is currently by getting the first available one like this.10. WorkerMessenger.unicast - windowClient
I looked at all the entry points of
WorkerMessenger.unicast
and where it gets awindowClient
passed in and it is handling it correctly.windowClient
comes fromclients.matchAll({ type: 'window', includeUncontrolled: true })
where only the first page with the correct URL is passed in.11. Additional files to check
An incomplete list of files that may have scope requirements that need to be checked.
Approach to getting OneSignal's instance of
ServiceWorkerRegistration
There are two options to get OneSignal's
ServiceWorkerRegistration
:await navigator.serviceWorker.register("https://mysite.com/path/OneSignalSDKWorker.js", {scope: "/push/onesignal/"});
await navigator.serviceWorker.getRegistration("/push/onesignal/")
No matter the implementation we should just have a simple getter for this to encapsulate it. Probably in
ServiceWorkerManager
orServiceWorkerHelper
Testing
After the required changes are made the following should be tested.
https://localhost/
and SW scoped tohttps://localhost/push/onesignal/
Migration
After we evaluate all OneSignal features work without requiring the page being under the SW's scope we have to make sure any changes made do not break any existing integrations using the root scope with OneSignal.
The text was updated successfully, but these errors were encountered: