Skip to content
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

v3 bgSync replayRequests unsupported #1248

Closed
DavidScales opened this issue Jan 26, 2018 · 18 comments
Closed

v3 bgSync replayRequests unsupported #1248

DavidScales opened this issue Jan 26, 2018 · 18 comments

Comments

@DavidScales
Copy link

DavidScales commented Jan 26, 2018

Library Affected:
workbox-backgroundSync

Browser & Platform:
Google Chrome v63.0.3

I'm trying to set up background sync with Workbox v3. After #1222, I have basic functionality, and I've added the queueDidReplay callback to show a notification when bg sync occurs:

const showNotification = () => {
  self.registration.showNotification('Background sync success!', {
    body: '🎉`🎉`🎉`'
  });
};

const bgSyncPlugin = new workbox.backgroundSync.Plugin(
  'dashboardr-queue',
  {
    callbacks: {
      queueDidReplay: showNotification
    }
  }
);

const networkWithBackgroundSync = new workbox.strategies.NetworkOnly({
  plugins: [bgSyncPlugin],
});

const addEventRoute = new workbox.routing.Route(
  ({url}) => url.pathname === '/api/add',
  networkWithBackgroundSync,
  'POST'
);

workbox.routing.registerRoute(addEventRoute);

This currently works, but bg sync takes about 5 minutes to happen in Chrome, and I'd like to replay the queued requests immediately. To achieve this I tried calling

bgSyncPlugin.replayRequests()

But this doesn't appear to work, as replayRequests is not a method of backgroundSync.Plugin:

Uncaught (in promise) TypeError: bgSyncPlugin.replayRequests is not a function
    at Object.replayAndReturn [as handle] (sw.js:110)
    at DefaultRouter.handleRequest (workbox-routing.dev.js:365)
    at workbox.routing.self.addEventListener.event (workbox-routing.dev.js:817)

So I changed backgroundSync.Plugin to backgroundsync.Queue, and that allows me to call replayRequests. But after switching to backgroundsync.Queue, failed requests are no longer being queued, and I'm not getting any console errors to indicate what's going wrong.

I understand that I might not be able to just swap Plugin & Queue interchangably, but if using Queue is not that solution, then how would I implement this functionality with Plugin? If Queue is the answer, what am I doing wrong?

Separate but related issues:
#1247 & #1249

@prateekbh
Copy link
Collaborator

@philipwalton should we just expose replayRequests from the plugin as well?

@philipwalton
Copy link
Member

@philipwalton should we just expose replayRequests from the plugin as well?

I don't think so. The reason we originally changed the Plugin class to not inherit from the Queue class was to avoid unintentional naming clashes. I think it mixes concerns too much to have plugin classes implement anything other than plugin lifecycle methods.

It sounds to me like @DavidScales should be using the Queue class directly and creating his own fetchDidFail method in an object to pass as the plugin (the same way the Plugin class does currently).

@DavidScales
Copy link
Author

Thanks for the replies.

@philipwalton can I request documentation updates then? I'm not sure how to go about doing that with the current v3 backgroundSync guide or modules documention.

To me the documentation suggests that the Queue example

const queue = new workbox.backgroundSync.Queue('myQueueName');
const request = new Request('/path/to/api', {
  method: 'POST',
  body: someformData,
});

try {
  await fetch(request);
} catch (err) {
  queue.addRequest(request);
}

seems like what I would want to do if I just want to create a queue for requests I'm constructing manually in my main thread (perhaps I'm not even using service workers).

While the Plugin example (above), seems like its exactly what I'm looking for based on the description

In addition to the Queue class, Workbox Background Sync also provides a Plugin class, so it can easily integrate with Workbox strategy classes that takes plugins as options, as well as Workbox routes, which respond to requests using Workbox strategies.

and the fact that it flows with my other SW code & works almost completely (only replayRequests is failing).

I could be misunderstanding something. I'm not super familiar with what separates a Plugin from the rest of the classes. But if the distinction is not what I'm describing above then I'm not sure how to know when I would use the Plugin over the Queue and vice versa.

@gauntface
Copy link

gauntface commented Jan 29, 2018

I didn't meant to push this to master, but to bring bg sync docs in line with the rest of the module.

Changes are here: google/WebFundamentals@112812d

@DavidScales does that kind of solve the doc issue or is there more?

@philipwalton is this ok with you?

@DavidScales
Copy link
Author

@gauntface sorry but I still don't see why my original posted code shouldn't work, based on the updated documentation. And I don't see why I would use the Queue class directly like @philipwalton is suggesting, nor how I would do so to implement my desired behavior 😕

The Using Workbox Background Sync with other Workbox Packages example still seems like exactly what I'm looking for, and works as described, except for replayRequests().

@gauntface
Copy link

Your original code works for me: https://quixotic-quit.glitch.me/

@DavidScales
Copy link
Author

An

Uncaught TypeError: workbox.claimClients is not a function

error causes the sw registration to fail

TypeError: Failed to register a ServiceWorker: ServiceWorker script evaluation failed

But yeah my code above works - what doesn't work is then calling bgSyncPlugin.replayRequests()

@gauntface
Copy link

I think I fixed the demo, but Chrome is showing:

[Violation] Only request notification permission in response to a user gesture.
(anonymous) @ (index):23

and Firefox doesn't support Sync but will show notification intermittently.

@DavidScales
Copy link
Author

Yeah, for sure. To re-iterate/summarize, I know that

  • my above code works, except for replayRequests
  • replayRequests isn't supported in Plugin

Given that, my questions are:

  1. Why doesn't the replayRequest API exist on Plugin?

In addition to the Queue class, Workbox Background Sync also provides a Plugin class, so it can easily integrate with Workbox strategy classes that takes plugins as options, as well as Workbox routes, which respond to requests using Workbox strategies.

  1. If we are certain that Queue should be used, how would I use it to implement my desired functionality?
  • I'm currently trying based on the existing documentation and can't figure out how - as noted in my original post I am not getting any error logs when attempting to use Queue.
  • Whatever the resolution, I'd request documentation updates. If I'm confused about how and why to use Queue, other developers might be as well.

If I'm just out in left field and not making any sense, I'm always open to hearing that as well 😉

@gauntface
Copy link

@DavidScales Can you open a bug with code example on the Queue stuff and keep this issue focused on the Plugin stuff. I'm just getting pretty lost with the amount of code going around in this issue and what is the actual problem.

Why not expose replayRequests() on the Plugin. The answer is simply - that's not something you should need to do. What is the use case for wanting it?

@DavidScales
Copy link
Author

DavidScales commented Jan 31, 2018

okay I've created an issue for getting Queue to work #1266

@gauntface
Copy link

@DavidScales I think the plugin is what you want - you just want a way to Force a sync event - i.e. work with DevTools. I'll dig into what you can use for the code lab - but adding the API isn't the way forward IMO.

@gauntface
Copy link

gauntface commented Feb 1, 2018

Ok so it looks like you can just use the sync button in Chrome DevTools to force everything to kick off - I didn't have to set a tag or anything.

So flow was

  1. Run gulp demos (which is the server) on Workbox
  2. Go to demo and click button to make requests.
  3. Kill server (ctrl+c on gulp demos)
  4. Click button to make more requests
  5. Check the requests are failing and getting queued in IndexedDB
  6. Start up server (run `gulp demos)
  7. Press the sync button in DevTools
  8. See network requests go through and check indexedDB is empty

screenshot from 2018-02-01 11-34-30

@gauntface
Copy link

gauntface commented Feb 2, 2018

@DavidScales I've added a section to the bottom of the background sync stuff: https://developers.google.com/web/tools/workbox/modules/workbox-background-sync

Is this enough to do what you want?

@DavidScales
Copy link
Author

@gauntface the documentation change is very helpful thanks! 👍

And I'm convinced of switching over to DevTools like you've suggested.

From my standpoint we can close this issue, everything unresolved is alive in #1266

However, (this possibly warrants a separate issue?) I'm not able to run the gulp demos you're mentioning. From the workbox repo, v3 branch, running gulp demos (after npm install), throws:

[12:49:38] Using gulpfile ~/Documents/tests/workbox/gulpfile.js
[12:49:38] Starting 'demos'...
[12:49:38] Starting 'demos:serve:local'...
[12:49:38] Starting 'demos:groupBuildFiles'...
[12:49:38] Finished 'demos:groupBuildFiles' after 835 ms
[12:49:38] Starting 'demos:firebaseServe:local'...

> workbox@0.0.0 demos-serve /Users/dscales/Documents/tests/workbox
> firebase serve --only hosting,functions --project workbox-demos


Error: Unable to authorize access to project workbox-demos
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! workbox@0.0.0 demos-serve: `firebase serve --only hosting,functions --project workbox-demos`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the workbox@0.0.0 demos-serve script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/dscales/.npm/_logs/2018-02-05T20_49_44_048Z-debug.log
[12:49:44] 'demos:firebaseServe:local' errored after 5.18 s
[12:49:44] Error: Error 1 returned from npm run,demos-serve
    at formatError (/Users/dscales/Documents/tests/workbox/node_modules/gulp/node_modules/gulp-cli/lib/versioned/^4.0.0/format-error.js:20:10)
    at Gulp.<anonymous> (/Users/dscales/Documents/tests/workbox/node_modules/gulp/node_modules/gulp-cli/lib/versioned/^4.0.0/log/events.js:31:15)
    at emitOne (events.js:121:20)
    at Gulp.emit (events.js:211:7)
    at Object.error (/Users/dscales/Documents/tests/workbox/node_modules/undertaker/lib/helpers/createExtensions.js:61:10)
    at handler (/Users/dscales/Documents/tests/workbox/node_modules/now-and-later/lib/mapSeries.js:43:14)
    at f (/Users/dscales/Documents/tests/workbox/node_modules/once/once.js:25:25)
    at f (/Users/dscales/Documents/tests/workbox/node_modules/once/once.js:25:25)
    at done (/Users/dscales/Documents/tests/workbox/node_modules/async-done/index.js:24:15)
    at _combinedTickCallback (internal/process/next_tick.js:135:11)
[12:49:44] 'demos:serve:local' errored after 6.02 s
[12:49:44] 'demos' errored after 6.02 s

Possibly a Firebase permissions thing?

@gauntface
Copy link

@DavidScales thats because the joy that is Firebase hosting + functions requires you to be logged in and have access to the project - regardless of local testing or actually deploying :(

I'm going to close this issue now if everything seems patched up in docs.

@DavidScales
Copy link
Author

ah ok. SGTM. thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants