GoogleChrome / workbox

📦 Workbox: JavaScript libraries for Progressive Web Apps
https://developers.google.com/web/tools/workbox/
MIT License
12.26k stars 804 forks source link

Background sync fallback in Safari not working #2386

Closed Adam-Reeves closed 4 months ago

Adam-Reeves commented 4 years ago

Library Affected: workbox-background-sync

Browser & Platform: Safari 13

Issue or Feature Request Description: The fallback for browsers that don't support background sync doesn't appear to be working as expected in a PWA I am working on.

When offline, the request is added to the queue and I can see this in IndexedDB. However no event other than installing a new service worker (a solution that's not workable in practice) will trigger the requests in the queue to be retried.

Looking in Queue.ts in workbox/packages/workbox-background-sync/src I can see on lines 413-415:

      // If the browser doesn't support background sync, retry
      // every time the service worker starts up as a fallback.
      this._onSync({queue: this});

this._onSync() only appears to be called the first time this._addSyncListener() is called but as it's not attached to an event listener it's never called again which would explain why it doesn't work as I expected.

As a workaround I could make use of the replayRequests() function but before doing so I would like to confirm whether or not this behaviour is expected.

Here's the service worker code (more or less a straight copy and paste from the Workbox docs):

  // Clone the request to ensure it's safe to read when
  // adding to the Queue.  
    const promiseChain = fetch(event.request.clone())
      .catch((err) => {
        self.clients.matchAll().then(all => all.map(client => client.postMessage("offline")));

        if(typeof BroadcastChannel !== "undefined") {
            const broadcastChannel = new BroadcastChannel('wb_channel');
            broadcastChannel.postMessage('offline');
        }
        return queue.pushRequest({ request: event.request });
      });
    event.waitUntil(promiseChain);
});
jeffposnick commented 4 years ago

Here's my understanding; CC: @philipwalton to investigate further.

Inside of the constructor for the Queue class, there's a call to this._addSyncListener():

https://github.com/GoogleChrome/workbox/blob/ce30f36ba192d9e2cbde55365c2c321893b1bde7/packages/workbox-background-sync/src/Queue.ts#L115

Inside of _addSyncListener(), there's a check for background sync support. In browsers like Safari, where there's no support, this._onSync() should end up being called unconditionally:

https://github.com/GoogleChrome/workbox/blob/ce30f36ba192d9e2cbde55365c2c321893b1bde7/packages/workbox-background-sync/src/Queue.ts#L409-L416

This constructor, and therefore the eventual this._onSync() call, should run once per instance of the Queue class in your service worker code, each time the service worker starts up. (The BackgroundSyncPlugin has an internal instance of the Queue class.)

Adam-Reeves commented 4 years ago

Hi Jeff,

Could you clarify what you mean when you say 'each time the service worker starts up' ? As in the 'install' phase of the lifecycle of a service worker?

The issue I'm having lies in the fact that this._onSync() is only called once per instance like you said, meaning the queued requests never get re-sent during the lifecycle of that particular service worker.

If this is expected functionality then that's fine as I can work around it. I thought it best to check first though :)

jeffposnick commented 4 years ago

There's more info at https://stackoverflow.com/a/38835274/385997

In general, a service worker starts and stops pretty frequently—unless, for some reason, you were doing something in a client page that would keep it alive indefinitely, like making a network request every N seconds.

One caveat is that some browsers (Chrome definitely does; not sure about Safari) will keep a service worker running indefinitely when you have DevTools open. You can read more about that behavior at https://stackoverflow.com/a/42741722/385997

Adam-Reeves commented 4 years ago

Thanks for the info.

Just to be on the safe side I have re-tested with DevTools closed in Safari.

It still doesn't look this._onSync() is getting called at any point after the service worker has been installed.

nip10 commented 4 years ago

Hi. Sorry for hijacking the issue, not sure whether to create a new one or add to this one.

I'm seeing a similar behavior when running a web app in Android WebView (Android v7, webview implementation: Chrome Stable).

I've done some debugging using DevTools (w/ Remote Debug) and I can see the requests being added to the queue (in IndexedDB), followed by a warning Unable to register sync event for 'myQueueName'. DOMException: Background Sync is disabled. which is expected since WebView doesn't support background sync. The problem is that the queue is never cleared/accessed.

I've tried waiting for long periods of time when reconnecting the device, restarting the browser, run with DevTools open/closed, and the requests just keep getting added to the queue.

Edit - More details: workbox v5.0.0; background sync plugin "basic" config; POST request; both client and server on the same domain using https. here's the sw code. Working in Chrome v80 (both Windows and Android), Firefox v68.5.0 (Android) and Firefox v73.0.1 (Windows). The weird thing is that Firefox works fine (even without the Background Sync API) showing Background sync replaying without background sync event

philipwalton commented 4 years ago

@nip10 can you open a separate issue for this? I think there might be a way for Chrome (with BG sync disabled) to fall back to the behavior used in Firefox and Safari.

Right now we feature detect if ('sync' in self.registration) for the reply logic, but your experience shows that we should probably make it conditional on the registration succeeding (and using the fallback when it doesn't).

Adam-Reeves commented 4 years ago

@philipwalton so will the fix of making the reply logic conditional on the registration succeeding fix the issues I'm having with Safari?

jszczypk commented 4 years ago

I am testing with Safari Desktop 13.1.2 and found out that fallback to call _onSync method to be called for sure only when browser starts. In my setup I have PouchDB replicating in the background all the time, so most likely Safari can't stop service worker as long as the page is opened.

My suggestion would be to provide a way to call currently private method Queue._onSync manually and even a way to access BackgroundSyncPlugin created Queue from outside. We could then check if Queue is full, show notification to user about stored requests and give them possibility to force synchronization manually. Another possibility would be to somehow use window.online event to inform Queue that it should replay.

WonderPanda commented 2 years ago

Any chance there are some updates here that would allow us to interact with the queue manually in the case that background sync isn't supported as suggested by @jszczypk?

jeffposnick commented 2 years ago

https://github.com/GoogleChrome/workbox/pull/2955 exposes the QueueStore and StorableRequest classes publicly, and https://github.com/GoogleChrome/workbox/pull/2941 exposes the size() of the QueueStore used by a Queue. Both of those PRs should be part of the upcoming Workbox v6.4.0 release.

I'm going to be honest in that I have never had to work with those pieces of workbox-background-syncmanually, so I'm not complete sure if it addresses the use case you detail here. @tropicadri has worked with this portion of the Workbox codebase more recently and might have some insight to share.

patrickcorrigan commented 1 year ago

I'm also experiencing the same issue @jszczypk. It seems to only be called when opening the browser. Did you end up solving the problem?

tomayac commented 4 months ago

Hi there,

Workbox is moving to a new engineering team within Google. As part of this move, we're declaring a partial bug bankruptcy to allow the new team to start fresh. We realize this isn't optimal, but realistically, this is the only way we see it working. For transparency, here're the criteria we applied:

Thanks, and we hope for your understanding! The Workbox team