Skip to content
This repository has been archived by the owner on Dec 9, 2021. It is now read-only.

Adds a new controlledBySW method #54

Merged
merged 6 commits into from Mar 28, 2017

Conversation

jeffposnick
Copy link
Contributor

R: @gauntface

Fixes #53

Copy link
Contributor

@gauntface gauntface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good - just please add a comment to the skipWaiting service worker please.

Will publish a release once it's merged.

@@ -0,0 +1,2 @@
self.addEventListener('install', () => self.skipWaiting());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment here just along the lines of:

"This service worker does not use event.waitUntil() on purpose. This has ambiguity over implementation across browsers (i.e. whether it's needed to be used with skipWaiting and clients.claim()). This service worker is used to test controlledBySw() - PLEASE DO NOT CHANGE."

Without this I can see someone eventually sneaking in event.waitUntil() (i.e. myself) and then it's undermines the purpose of this new method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the ambiguity is, though? AFAIK there are no browsers that ever required an event.waitUntil() wrapper around self.skipWaiting() or self.clients.claim().

There are old examples that used event.waitUntil() (including many I wrote), but it turns out it was never actually necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I added a comment anyway!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we've seen the difference in behavior - so it's not "required" it does affect the end behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL at the revised comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that works.

@jeffposnick
Copy link
Contributor Author

I've made eslint happier with some of the latest commits, but the actual tests are failing now with

WebDriverError: Unable to parse new session response: {"value": {"sessionId":"e8da8745-f4ac-46c5-ba1f-de5b36a55a4d","value":{"acceptInsecureCerts":false,"browserName":"firefox","browserVersion":"55.0a1","moz:accessibilityChecks":false,"moz:processID":7046,"moz:profile":"/tmp/rust_mozprofile.RHnTV5h5iha9","pageLoadStrategy":"normal","platformName":"linux","platformVersion":"4.8.12-040812-generic","rotatable":false,"specificationLevel":0,"timeouts":{"implicit":0,"pageLoad":300000,"script":30000}}}}

I'm going to add in another commit that bumps all the devDependenciesand see if that helps, unless you have other ideas, @gauntface.

@jeffposnick
Copy link
Contributor Author

Updating the dependencies is pretty involved, unfortunately, so I'll tackle it in a separate PR that will need to be merged first.

@jeffposnick
Copy link
Contributor Author

This PR is blocked on #55, as I haven't had any luck getting the test suite to run properly in Firefox 😦

@gauntface
Copy link
Contributor

@jeffposnick if you can switch back to let instead of disabling the lint check, I'll merge the PR and will tackle the tests in a follow up PR if you like.

@jeffposnick
Copy link
Contributor Author

Oops—my forked copy was out of date with the upstream master, and I didn't realize that the rest of that file had switched from var to let. I couldn't figure out why linting was unhappy with just that new line... it's switched to let now.

@gauntface
Copy link
Contributor

@jeffposnick Tank you :) Will make sure lint is at least happy + Chrome and then will merge. Sorry for the faff.

@gauntface gauntface merged commit 87563c5 into GoogleChromeLabs:master Mar 28, 2017
@jeffposnick jeffposnick deleted the controlled-by-sw branch April 19, 2017 14:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants