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

Multiple controlling events #2817

Closed
joshkel opened this issue Apr 14, 2021 · 2 comments · Fixed by #2857
Closed

Multiple controlling events #2817

joshkel opened this issue Apr 14, 2021 · 2 comments · Fixed by #2857
Labels
Bug An issue with our existing, production codebase. workbox-window

Comments

@joshkel
Copy link
Contributor

joshkel commented Apr 14, 2021

Library Affected: workbox-window

Browser & Platform: Tested on Chrome 89.0.4389.114 for Mac

Issue or Feature Request Description:

I'm having trouble getting the Workbox class's controlling event to work like I expect - or maybe I'm misunderstanding how it's expected to work.

Scenario: I want my page to reload on update, like in the recipe:

wb.addEventListener('controlling', (event) => {
  window.location.reload();
});

However, unlike the recipe, I want my page to reload even if the update was confirmed in the UI in another browser tab. So I set up this event handler at application start, instead of in response to a waiting event. (That was working even prior to #2786 and Workbox 6.1.5, so I may be misunderstanding #2786.)

Then I noticed that my page would reload when it was first loaded (because the newly installed service worker takes control via clientsClaim). That seems unnecessary, so I added a check for that:

wb.addEventListener('controlling', (event) => {
  if (!event.isUpdate) {
    window.location.reload();
  }
});

However, I then discovered that the controlling event can only fire once: a newly loaded page gets the controlling event for the initial load, so it then no longer gets events notifying of future updates.

This seems like a bug, but I could easily be misunderstanding something. Is this the desired behavior?

@jeffposnick
Copy link
Contributor

Thanks for catching that, @joshkel. My supposition is that

navigator.serviceWorker.addEventListener(
'controllerchange', this._onControllerChange, {once: true});

was written with {once:true} originally, as an optimizing for when there was no concept of "external" controlling events in workbox-window, and that we should have removed the {once: true} when we added support for all types of controlling events, regardless of whether they originated from the page's original service worker, or a new service worker on a different page.

I think you're already familiar with our codebase, so if you want to take on removing that {once: true}, you are encouraged to! Otherwise, we can.

@jeffposnick jeffposnick added Bug An issue with our existing, production codebase. workbox-window labels Apr 20, 2021
rockwalrus added a commit to StateFarmIns/workbox that referenced this issue May 13, 2021
Dispatches the `controlling` event every time the documentation 
specifies that it should.
jeffposnick added a commit that referenced this issue Jul 1, 2021
* Fire controlling event more than once (#2817)

Dispatches the `controlling` event every time the documentation 
specifies that it should.

* Test tweak

* One more fix

Co-authored-by: Jeff Posnick <jeffy@google.com>
@jeffposnick
Copy link
Contributor

The fix is now deployed in a pre-release of Workbox v6.2.0: https://github.com/GoogleChrome/workbox/releases/tag/v6.2.0-alpha.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug An issue with our existing, production codebase. workbox-window
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants