search

OneSignal / OneSignal-Website-SDK

OneSignal is a push notification service for web and mobile apps. This SDK makes it easy to integrate your website with OneSignal Push Notifications. https://onesignal.com
Other
383 stars 114 forks source link

Investigate removing page controller requirement for the ServiceWorker #615

Closed jkasten2 closed 6 months ago

jkasten2 commented 4 years ago

Current State

Update 2021-02-24:

See latest state in this comment: https://github.com/OneSignal/OneSignal-Website-SDK/issues/615#issuecomment-785611442

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.

navigator.serviceWorker.register("...").then(reg => {
  reg.active.postMessage("greet_from_index");`
}

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 use navigator.serviceWorker.ready so these lines need to be updated.

3. clients.matchAll({ type: 'window', includeUncontrolled: false })

Status - Tested with Chrome, works as intended - includeUncontrolled

One spot where we use includeUncontrolled: false instead of true, looks safe to make this true but need do additional testing. Specifically need to ensure on_focus and on_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.

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 this

6. 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.

8. skipWaiting & claim()

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 on ServiceWorkerRegistration.active only as it might be null. 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.

const availableWorker = serviceWorker.active || serviceWorker.waiting || serviceWorker.installing;

10. WorkerMessenger.unicast - windowClient

I looked at all the entry points of WorkerMessenger.unicast and where it gets a windowClient passed in and it is handling it correctly. windowClient comes from clients.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:

  1. Register it
    • await navigator.serviceWorker.register("https://mysite.com/path/OneSignalSDKWorker.js", {scope: "/push/onesignal/"});
    • We could do this every time, not sure if there are any downsides to registering again if it is already there.
  2. Get worker at scope
    • 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 or ServiceWorkerHelper

Testing

After the required changes are made the following should be tested.

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.

westonruter commented 4 years ago

In an AMP context, the eliminating of the controller requirement will allow for amp-web-push to be added for OneSignal regardless of whether a page already has an amp-install-serviceworker component present.

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>
jkasten2 commented 4 years ago

@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 helper-iframe-url, permission-dialog-url, service-worker-url, and service-worker-scope matches or if service-worker-scope is a higher scope than the other 3 then the expected features will work for AMP.

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 amp-web-push is doing to avoid the root scope service worker issue. However it be much simpler to use the workaround noted above, which is for the developer to create a single service worker with importScripts to merge multiple server worker files together.

jeffposnick commented 4 years ago

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 message event that wakes up the OneSignal service worker. If developers need to force an update to happen, due to a bug fix or something else, passing in an updated URL to navigator.serviceWorker.register() would be necessary.

westonruter commented 4 years ago

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.

jkasten2 commented 3 years ago

@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 ServiceWorker, if page control isn't required.

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 my testing in Edge(Chromium), Chrome, Firefox, and Safari(macOS) this always resulted in the SW getting the post message. Even if reg.installing ended up being the available instance the SW got the message so there doesn't seem to be a 'not ready" issue here.

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. I'll follow up with you if I have any more ServiceWorker questions and update you when we ship this to production.

jeffposnick commented 3 years ago

const availableWorker = reg.installing || reg.waiting || reg.active;, used in that context, should reliably give you access to an instance of the service worker.

The only thing to decide on is the order that you use in the ||. Just be aware that it's possible for all three of the installing, waiting, and active properties to be set to distinct ServiceWorker objects, if the user revisits that page after the service worker has been redeployed. (You could avoid the possibility of waiting being set if you use skipWaiting() in your service worker.)

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 reg.active first in your || sequence.

jkasten2 commented 3 years ago

Status Update

✅ Whats live and ready now

@jeffposnick @westonruter Site developers or (plugin developers who include OneSignal) can choose to customize the ServiceWorker scope with the following instructions: https://documentation.onesignal.com/docs/onesignal-service-worker-faq#typical-site-setup---service-worker-customizations This will work for those setting up OneSignal for the first time or want to migrate to a new scope. 🚧 Warning: We have only fleshed out scope changes, not href changes to the SW, more details below.

What was shipped details

The 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. Scope is fine to change but we are still hammering out the exact steps to safely migrate to a different href URL for the OneSignal ServiceWorker file for existing OneSignal setups.

  1. I have concerns about the SW code going stale if the developer removes the file but the browser never returns to their site. I found this issue that explains 4xx HTTP codes do NOT unregister the SW. We are considering the developer keep old ServiceWorker available to prevent this, at least for a while.
  2. Also OneSignal may want to provide a mechanize that the newer ServiceWorker is fully ready to take over (new push endpoint URL has been send to OneSignal API etc) so the web developer can choose to call ServiceWorkerRegistration.unregister

🚧Defaulting a Non-root scope for new setups

As 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 code

This 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. This could be done by simply fetching the contains of the configured OneSignal SW file (example: https://yoursite.com/OneSignalSDKWorker.js) and ensure it's contains are exactly the following (per OneSignal setup instructions):

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.

jeffposnick commented 3 years ago

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 Migration to different ServiceWorker Href concerns. But in general, you might be able to make sure of self.registration.scope and self.registration.unregister() inside of code you do control, like the SDK, if that's helpful to your ends.

jkasten2 commented 3 years ago

@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. But a more nuance caveats I discovered if a ServiceWorker was registered with a Service-Worker-Allowed HTTP header and the server no longer provides the header the browser won't accept the new SW code on a background event, such as a push. Basically the code SW code will go stale and never update. Make sense in hindsight but also quite important to keep the Service-Worker-Allowed header on the "old" ServiceWorker if you plan to make breaking push payload changes and assume all SWs are updated.

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.

  1. Service Workers: an Introduction
  2. The Service Worker Lifecycle
  3. Service Worker Registration - Nuances to navigator.serviceWorker.register
  4. Fresher service workers, by default - ServiceWorker update options
  5. MDN - Service Worker API
  6. W3C ServiceWorker Spec
    • I only used as reference to read a few specifics. (I am web developer not a browser dev 🙃)
  7. Service worker JavaScript update frequency (every 24 hours?)
    • @jeffposnick Excellent write up here! Wasn't able to find out some of these details anywhere else.

Also if you have any other good SW docs in general send them my way too.

jeffposnick commented 3 years ago

That's interesting—I've never had to use Service-Worker-Allowed, so I wasn't aware of that nuance around keeping the header whenever you serve the response. Another relevant header is Service-Worker: script, which is sent on all requests that a browser makes for a service worker (either in the initial registration, or in an update check). That can be handy if you have full control over the web server and want to respond to any request for a service worker, regardless of the service worker URL, with a valid response.

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.

jkasten2 commented 3 years ago

@jeffposnick Ah interesting, didn't know about the browser putting service-worker in the request HTTP header. Thanks!

maxhartshorn commented 3 years ago

@jkasten2 What's the status on this issue? Is it still blocking compatibility between OneSignal and the Wordpress PWA plugin?

jkasten2 commented 3 years ago

@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.

westonruter commented 3 years ago

@jkasten2 Excellent! So that means AMP compatibility should now be doable by serving an amp-web-push component on AMP pages (when amp_is_request() is true) as opposed to the current JS installation method in OneSignal_Public?

jkasten2 commented 3 years ago

@westonruter Since there won't be a service worker scope to avoid then I believe determining if you include amp-web-push on the page or not could be driven by amp_is_request(). The attributes of amp-web-push would still be dependent on which push vendor you are using though.

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?

westonruter commented 3 years ago

I'm no expert on the amp-web-push component, but there is usage examples on the component docs page. Namely, it is configured with a custom element like so:

<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 <amp-web-push>. See an example in the Newspack plugin's yet-unmerged PR https://github.com/Automattic/newspack-plugin/pull/417 which seeks to add AMP-compatible push notifications. The amp-web-push attributes could be set accordingly:

So when amp_is_request() returns true, then that component could be rendered on the page. If false (or the function is not defined), then the plugin could continue doing what it does at present in OneSignal_Public::onesignal_header().

See also https://github.com/OneSignal/OneSignal-WordPress-Plugin/issues/258.

jkasten2 commented 3 years ago

@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. https://github.com/OneSignal/OneSignal-WordPress-Plugin/issues/272

jkasten2 commented 6 months ago

Closing this issue, changes to address the service worker page control were fixed in PR #745 and released in version 151300