Implementing forced nullification #25

prateekbh · 2019-04-24T20:37:34Z

implements a kill switch worker method installNoOpServiceWorker which removes the currently installed service worker.
claim all clients
force refreshes clients once, after installation
cleans all known amp-sw caches
does not intercept any call

…ll-switch

prateekbh · 2019-04-24T20:38:39Z

src/modules/core/index.ts

 // Initialize AMP_SW namespace
 self['AMP_SW'] = {
  init,
+  installNoOpServiceWorker,


I can really use some help in deciding a better name

kristoferbaxter · 2019-05-23T21:25:54Z

src/builder/index.ts

@@ -23,6 +23,7 @@ export async function buildSW(
    documentCachingOptions: {},
  },
  importFrom: string = 'https://cdn.ampproject.org/amp-sw.js',
+  eject: boolean = false,


Let's use the same name everywhere, called eject here and installNoOpServiceWorker below.

Pick one!

Also, this function could use some documentation.

kristoferbaxter · 2019-05-23T21:29:23Z

src/builder/index.ts

@@ -32,6 +33,11 @@ export async function buildSW(
    );


config.offlinePageOptions.assets = (config.offlinePageOptions.assets || []).concat(...

just curious, how is this more readable than what is there today?

kristoferbaxter · 2019-05-23T21:29:51Z

src/builder/index.ts

+  } else {
+    code += `AMP_SW.init(${serializeObject(config || {})})`;
+  }
+
  return code;
 }


Let's move this all to testing directories, since it's purely for test cases.

Will do this in a separate PR, issue opened #30

kristoferbaxter · 2019-05-23T21:30:31Z

src/builder/index.ts

+    code += 'AMP_SW.installNoOpServiceWorker()';
+  } else {
+    code += `AMP_SW.init(${serializeObject(config || {})})`;
+  }


Can just return code + '' instead of conditionals.

kristoferbaxter · 2019-05-23T21:31:57Z

src/modules/core/index.ts

 declare global {
  interface WorkerGlobalScope {
    AMP_SW: {
      init: Function;
+      installNoOpServiceWorker: Function;


Can we add more explicit typing for what the installNoOpServiceWorker function is?

kristoferbaxter · 2019-05-23T21:33:12Z

src/modules/core/index.ts

+  import(/* webpackChunkName: "service-worker-remover" */ '../service-worker-remover/index').then(
+    async ({ ServiceWorkerRemover }) => {
+      const swRemover = new ServiceWorkerRemover();
+      swRemover.installNoOpServiceWorker();


Not using the value elsewhere, so you can just...

new ServiceWorkerRemover().installNoOpServiceWorker();

kristoferbaxter · 2019-05-23T21:34:30Z

src/modules/link-prefetch/constants.ts

@@ -0,0 +1 @@
+export const cacheName: string = 'AMP-PREFETCHED-LINKS';


Let's capitalize this constant, for consistency.

Also, you might consider calling it something more explicit since it's used outside of the link-prefetch component now.

kristoferbaxter · 2019-05-23T21:35:18Z

src/modules/offline-page/index.ts

@@ -14,8 +14,8 @@
 * limitations under the License.
 */

-import { cacheName as documentCache } from '../document-caching/constants';
-import { cacheName as assetCache } from '../asset-caching/constants';
+import { cacheName as AMP_PUBLISHER_CACHE_NAME } from '../document-caching/constants';


Nice to avoid the as statements during import by naming the cacheName this in the first place. Helps with finding usages.

kristoferbaxter · 2019-05-23T21:36:38Z

src/modules/offline-page/index.ts

-    const publisherCache = await caches.open(documentCache);
-    const assetsCache = await caches.open(assetCache);
+    const publisherCache = await caches.open(AMP_PUBLISHER_CACHE_NAME);
+    const assetsCache = await caches.open(ASSET_CACHE);


Looks like a good place to use Promise.all or similar, including the fetch in the next line.

kristoferbaxter · 2019-05-23T21:40:13Z

src/modules/service-worker-remover/index.ts

+import { cacheName as AMP_PREFETCHED_LINKS } from '../link-prefetch/constants';
+
+export class ServiceWorkerRemover {
+  async installNoOpServiceWorker() {


Let's verify the first version of Safari supporting Service Workers, there is a bug in Safari 10.1 that would preclude an async member from working as expected.

kristoferbaxter · 2019-05-23T21:42:09Z

src/modules/service-worker-remover/index.ts

+export class ServiceWorkerRemover {
+  async installNoOpServiceWorker() {
+    // Taking over the document
+    self.addEventListener('install', function(e: ExtendableEvent) {


Should these be onces?

This memory is freed once the service worker is terminated, and post that service worker execution is just event based so i guess this should matter.
Also there is not good place/lifecycle hook to clear these listeners

kristoferbaxter · 2019-05-23T21:43:12Z

src/modules/service-worker-remover/index.ts

+    return clients.claim().then(async () => {
+      // Cache current document if its AMP.
+      const windowClients = await clients.matchAll({ type: 'window' });
+      windowClients.forEach((client: WindowClient) => {


windowClients.forEach((client: WindowClient) => { client.navigate(client.url); });

windowClients.forEach((client: WindowClient) => client.navigate(client.url));

kristoferbaxter · 2019-05-23T21:44:31Z

src/modules/service-worker-remover/index.ts

+  }
+
+  async cleanCacheStorage() {
+    return Promise.all([


Can we build a test that verifies that an exception thrown in these methods doesn't break the SW?

This is a bit tough to do in the current e2e test setup due to the following reasons

The CacheStorage API never rejects the promise it just returns true/false

The service worker clears the cache and refreshes the window so even if I try to change the prototype to reject the promise, the service worker thread never gets the change.

Instead I can test this with a unit test case instead.

I'll setup a unit test setup and add a unit tests around service worker forced nullification in a separate PR.
It also makes sense to have these tests for forced nullification module because this piece is beyond just configuring workbox

kristoferbaxter · 2019-05-23T21:48:02Z

test/modules/service-worker-remover/service-worker-remover-test.js

+
+  it('should remove currently registered service worker', async () => {
+    const installedServiceWorker = await driver.executeAsyncScript(async cb => {
+      executeScript(async cb => {


executeScript(async cb => { cb(navigator.serviceWorker.controller.scriptURL); }, cb);

executeScript(async cb => cb(navigator.serviceWorker.controller.scriptURL), cb);

kristoferbaxter

See comments.

kristoferbaxter · 2019-06-12T20:57:42Z

src/modules/core/index.ts

@@ -96,7 +98,16 @@ function init(config: ServiceWorkerConfiguration = {}) {
  });
 }

+function forcedNullifcation() {
+  import(/* webpackChunkName: "service-worker-remover" */ '../service-worker-remover/index').then(
+    async ({ ServiceWorkerRemover }) => {


async ({ ServiceWorkerRemover }) => { new ServiceWorkerRemover().installNoOpServiceWorker(); },

Can be rewritten as...

async ({ ServiceWorkerRemover }) => new ServiceWorkerRemover().installNoOpServiceWorker(),

are you aware of a prettier rule for this?
This definitely looks like an optimization that should either be done by prettier or the compiler.

manually fixing this for now

prateekbh added 4 commits April 23, 2019 14:40

WIP kill switch implementation

bc2dd33

fixing tests

a2d3b05

adding tests for kill switch

5611ec0

Merge branch 'master' of https://github.com/ampproject/amp-sw into ki…

ae563bc

…ll-switch

prateekbh requested a review from kristoferbaxter April 24, 2019 20:37

prateekbh commented Apr 24, 2019

View reviewed changes

kristoferbaxter reviewed May 23, 2019

View reviewed changes

kristoferbaxter suggested changes May 23, 2019

View reviewed changes

prateekbh added 2 commits June 3, 2019 16:15

resolving comment partially

3e2e8a5

fixing tests

942033d

prateekbh requested a review from kristoferbaxter June 6, 2019 19:55

kristoferbaxter approved these changes Jun 12, 2019

View reviewed changes

prateekbh changed the title ~~Implementing a kill switch~~ Implementing forced nullification Jun 13, 2019

resolving comments

ad85749

prateekbh merged commit b376d0d into master Jun 13, 2019

prateekbh deleted the kill-switch branch June 13, 2019 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing forced nullification #25

Implementing forced nullification #25

prateekbh commented Apr 24, 2019 •

edited by kristoferbaxter

Loading

prateekbh Apr 24, 2019

prateekbh Jun 5, 2019

kristoferbaxter May 23, 2019 •

edited

Loading

prateekbh Jun 5, 2019

kristoferbaxter May 23, 2019

prateekbh Jun 5, 2019

kristoferbaxter May 23, 2019

prateekbh Jun 5, 2019

kristoferbaxter May 23, 2019

prateekbh Jun 5, 2019

kristoferbaxter May 23, 2019

prateekbh Jun 3, 2019

kristoferbaxter May 23, 2019

prateekbh Jun 3, 2019

kristoferbaxter May 23, 2019

prateekbh Jun 3, 2019

kristoferbaxter May 23, 2019

prateekbh Jun 3, 2019

kristoferbaxter May 23, 2019 •

edited

Loading

prateekbh Jun 3, 2019

kristoferbaxter May 23, 2019

prateekbh Jun 3, 2019

kristoferbaxter May 23, 2019

prateekbh Jun 5, 2019

kristoferbaxter May 23, 2019 •

edited

Loading

prateekbh Jun 3, 2019

kristoferbaxter May 23, 2019

prateekbh Jun 6, 2019

kristoferbaxter May 23, 2019

prateekbh Jun 3, 2019

kristoferbaxter left a comment

kristoferbaxter Jun 12, 2019

prateekbh Jun 12, 2019

prateekbh Jun 13, 2019

		@@ -0,0 +1 @@
		export const cacheName: string = 'AMP-PREFETCHED-LINKS';

Implementing forced nullification #25

Implementing forced nullification #25

Conversation

prateekbh commented Apr 24, 2019 • edited by kristoferbaxter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoferbaxter May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoferbaxter May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoferbaxter May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoferbaxter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekbh commented Apr 24, 2019 •

edited by kristoferbaxter

Loading

kristoferbaxter May 23, 2019 •

edited

Loading

kristoferbaxter May 23, 2019 •

edited

Loading

kristoferbaxter May 23, 2019 •

edited

Loading