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

Implementing forced nullification #25

Merged
merged 7 commits into from
Jun 13, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/builder/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export async function buildSW(
documentCachingOptions: {},
},
importFrom: string = 'https://cdn.ampproject.org/amp-sw.js',
forcedNullifcation: boolean = false,
) {
let code = '';
if (config.offlinePageOptions && config.offlinePageOptions.url) {
Expand All @@ -32,6 +33,9 @@ export async function buildSW(
);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}
code = `importScripts('${importFrom}')\n`;
code += `AMP_SW.init(${serializeObject(config || {})})`;
return code;
if (forcedNullifcation) {
return code + 'AMP_SW.forcedNullifcation()';
} else {
return code + `AMP_SW.init(${serializeObject(config || {})})`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can just return code + '' instead of conditionals.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

2 changes: 2 additions & 0 deletions src/modules/amp-caching/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export const UNVERSIONED_CACHE_NAME = 'AMP-UNVERSIONED-CACHE';
export const VERSIONED_CACHE_NAME = 'AMP-VERSIONED-CACHE';
3 changes: 1 addition & 2 deletions src/modules/amp-caching/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,11 @@ import { CacheFirst, StaleWhileRevalidate } from 'workbox-strategies';
import { Plugin } from 'workbox-cache-expiration';
import { FluxStandardAction } from '../flux-standard-actions';
import { AmpSwModule } from '../core/AmpSwModule';
import { UNVERSIONED_CACHE_NAME, VERSIONED_CACHE_NAME } from './constants';

const VERSIONED_ASSETS_RE = /^https:\/\/cdn.ampproject.org\/rtv\/\d*\//;
const UNVERSIONED_RUNTIME_RE = /^https:\/\/cdn.ampproject.org\/\w*(-\w*)?.js/;
const UNVERSIONED_EXTENSIONS_RE = /^https:\/\/cdn.ampproject.org\/v0\//;
const UNVERSIONED_CACHE_NAME = 'AMP-UNVERSIONED-CACHE';
const VERSIONED_CACHE_NAME = 'AMP-VERSIONED-CACHE';

async function cachePreRequestedScripts(scripts: Array<string>) {
const unversionedScripts: Array<Request> = [];
Expand Down
2 changes: 1 addition & 1 deletion src/modules/asset-caching/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
* limitations under the License.
*/

export const cacheName = 'AMP-ASSET-CACHE';
export const AMP_ASSET_CACHE = 'AMP-ASSET-CACHE';
4 changes: 2 additions & 2 deletions src/modules/asset-caching/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
} from 'workbox-strategies';
// @ts-ignore
import { Plugin } from 'workbox-cache-expiration';
import { cacheName } from './constants';
import { AMP_ASSET_CACHE } from './constants';
import { AmpSwModule } from '../core/AmpSwModule';

export type AssetCachingOptions = Array<{
Expand Down Expand Up @@ -75,7 +75,7 @@ export class AssetCachingAmpModule implements AmpSwModule {
assetCachingOptions.forEach(assetCachingOption => {
let cachingStrategy = null;
const cachingConfig = {
cacheName,
cacheName: AMP_ASSET_CACHE,
plugins: [
new AssetCachingPlugin({
maxEntries: 25,
Expand Down
11 changes: 11 additions & 0 deletions src/modules/core/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ import { ServiceWorkerConfiguration } from '../../configuration';
import { AssetCachingOptions } from '../asset-caching';
import { LinkPrefetchOptions } from '../link-prefetch';
import { OfflinePageOptions } from '../offline-page';

declare global {
interface WorkerGlobalScope {
AMP_SW: {
init: Function;
forcedNullifcation: () => void;
};
}
}
Expand Down Expand Up @@ -96,7 +98,16 @@ function init(config: ServiceWorkerConfiguration = {}) {
});
}

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

Choose a reason for hiding this comment

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

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

Can be rewritten as...

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

manually fixing this for now

new ServiceWorkerRemover().installNoOpServiceWorker();
},
);
}

// Initialize AMP_SW namespace
self['AMP_SW'] = {
init,
forcedNullifcation,
};
4 changes: 2 additions & 2 deletions src/modules/document-caching/AmpDocumentNetworkFirst.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

// @ts-ignore
import { NetworkFirst } from 'workbox-strategies';
import { cacheName } from './constants';
import { AMP_PUBLISHER_CACHE } from './constants';

export class AmpDocumentNetworkFirst extends NetworkFirst {
_offlineFallbackUrl?: string;
Expand All @@ -35,7 +35,7 @@ export class AmpDocumentNetworkFirst extends NetworkFirst {
}) {
let response = await super.makeRequest({ event, request });
if (!response && this._offlineFallbackUrl) {
const cache = await caches.open(cacheName);
const cache = await caches.open(AMP_PUBLISHER_CACHE);
response = await cache.match(this._offlineFallbackUrl);
}
return response;
Expand Down
2 changes: 1 addition & 1 deletion src/modules/document-caching/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
* limitations under the License.
*/

export const cacheName: string = 'AMP-PUBLISHER-CACHE';
export const AMP_PUBLISHER_CACHE: string = 'AMP-PUBLISHER-CACHE';
6 changes: 3 additions & 3 deletions src/modules/document-caching/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import router from 'workbox-routing';
// @ts-ignore
import { enable as enableNagigationPreload } from 'workbox-navigation-preload';
import { cacheName } from './constants';
import { AMP_PUBLISHER_CACHE } from './constants';
import AmpDocumentCachablePlugin from './AmpDocumentCachablePlugin';
import AmpNavigationRoute from './AmpNavigationRoute';
import { AmpDocumentNetworkFirst } from './AmpDocumentNetworkFirst';
Expand Down Expand Up @@ -73,7 +73,7 @@ export class DocumentCachingModule implements AmpSwModule {
const navRoute = new AmpNavigationRoute(
new AmpDocumentNetworkFirst(
{
cacheName,
cacheName: AMP_PUBLISHER_CACHE,
plugins: [
new AmpDocumentCachablePlugin({
maxEntries: documentCachingOptions.maxDocumentsInCache || 10,
Expand Down Expand Up @@ -109,7 +109,7 @@ export class DocumentCachingModule implements AmpSwModule {
const responseToBeCached = await ampCachablePlugin.cacheWillUpdate({
response,
});
const cache = await caches.open(cacheName);
const cache = await caches.open(AMP_PUBLISHER_CACHE);
if (responseToBeCached) {
cache.put(request, responseToBeCached);
}
Expand Down
1 change: 1 addition & 0 deletions src/modules/link-prefetch/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const AMP_PREFETCHED_LINKS: string = 'AMP-PREFETCHED-LINKS';
8 changes: 4 additions & 4 deletions src/modules/link-prefetch/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ import { FluxStandardAction } from '../flux-standard-actions';
import AmpNavigationRoute from '../document-caching/AmpNavigationRoute';
import { AmpPrefetchPlugin } from './AmpPrefetchPlugin';
import { AmpSwModule } from '../core/AmpSwModule';
import { AMP_PREFETCHED_LINKS } from './constants';

export type LinkPrefetchOptions = {
maxAgeSecondsInCache: Number;
};

const cacheName = 'AMP-PREFETCHED-LINKS';
let navigationRoute_: AmpNavigationRoute;
let linkPrefetchOptions_: LinkPrefetchOptions | undefined;

Expand Down Expand Up @@ -79,7 +79,7 @@ export class LinkPrefetchAmpModule implements AmpSwModule {
linkPrefetchOptions_ = linkPrefetchOptions;
navigationRoute_ = navigationRoute;
// Read all prefetched links and add it to deny list.
const cache = await caches.open(cacheName);
const cache = await caches.open(AMP_PREFETCHED_LINKS);
const linksRegExps: Array<RegExp> = [];
(await cache.keys()).forEach(request => {
let url = request.url;
Expand Down Expand Up @@ -107,7 +107,7 @@ export class LinkPrefetchAmpModule implements AmpSwModule {
router.registerRoute(
link,
new CacheFirst({
cacheName,
cacheName: AMP_PREFETCHED_LINKS,
plugins: [
new AmpPrefetchPlugin({
maxEntries: 10,
Expand All @@ -134,7 +134,7 @@ export class LinkPrefetchAmpModule implements AmpSwModule {
});
// TODO: add logic to only allow URLs within SW scope.
if (allowedLinks && allowedLinks.length > 0) {
const cache = await caches.open(cacheName);
const cache = await caches.open(AMP_PREFETCHED_LINKS);
await cache.addAll(allowedLinks);
const linkRegExps = allowedLinks.map(link =>
convertUrlToRegExp(cleanHostInfoFromUrl(link)),
Expand Down
10 changes: 6 additions & 4 deletions src/modules/offline-page/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 { AMP_PUBLISHER_CACHE } from '../document-caching/constants';
import { AMP_ASSET_CACHE } from '../asset-caching/constants';
import { AmpSwModule } from '../core/AmpSwModule';

export type OfflinePageOptions = {
Expand All @@ -25,8 +25,10 @@ export type OfflinePageOptions = {

export class OfflinePageAmpSwModule implements AmpSwModule {
async init(url: string, assets: Array<string>) {
const publisherCache = await caches.open(documentCache);
const assetsCache = await caches.open(assetCache);
const [publisherCache, assetsCache] = await Promise.all([
caches.open(AMP_PUBLISHER_CACHE),
caches.open(AMP_ASSET_CACHE),
]);
const response = await fetch(url);
if (response.ok) {
await publisherCache.put(new Request(url), response);
Expand Down
63 changes: 63 additions & 0 deletions src/modules/service-worker-remover/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/**
* Copyright 2019 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import {
VERSIONED_CACHE_NAME,
UNVERSIONED_CACHE_NAME,
} from '../amp-caching/constants';
import { AMP_ASSET_CACHE } from '../asset-caching/constants';
import { AMP_PUBLISHER_CACHE } from '../document-caching/constants';
import { AMP_PREFETCHED_LINKS } from '../link-prefetch/constants';

export class ServiceWorkerRemover {
async installNoOpServiceWorker() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

11.2

// Taking over the document
self.addEventListener('install', function(e: ExtendableEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be onces?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

const { skipWaiting } = self as ServiceWorkerGlobalScope;
e.waitUntil(skipWaiting());
});

self.addEventListener('activate', async (e: ExtendableEvent) => {
const { clients } = self as ServiceWorkerGlobalScope;
e.waitUntil(
Promise.all([
this.cleanCacheStorage(),
this.forceRefreshClients(clients),
]),
);
});
}

async cleanCacheStorage() {
return Promise.all([
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

caches.delete(VERSIONED_CACHE_NAME),
caches.delete(UNVERSIONED_CACHE_NAME),
caches.delete(AMP_ASSET_CACHE),
caches.delete(AMP_PUBLISHER_CACHE),
caches.delete(AMP_PREFETCHED_LINKS),
]);
}

forceRefreshClients(clients: Clients) {
return clients.claim().then(async () => {
// Cache current document if its AMP.
const windowClients = await clients.matchAll({ type: 'window' });
windowClients.forEach((client: WindowClient) =>
client.navigate(client.url),
);
});
}
}
1 change: 1 addition & 0 deletions test/amp_metadata.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"ampRuntimeVersion":"011905140117570","ampCssUrl":"https://cdn.ampproject.org/rtv/011905140117570/v0.css","canaryPercentage":"0.015","diversions":["001905222334000","031905222334000","021905140117570"]}
Loading