ampproject / amp-sw Goto Github PK

No reason for this to be public.

In reviewing the link-prefetch module, I realized there is quite a bit of overlap with Quicklink. The link-prefetch module will prefetch links in the page that have the data-prefetch data-rel=prefetch attribute:

https://github.com/ampproject/amphtml/blob/0937333cb3f4d1b09bd41f86db565c2dcda7ed3a/extensions/amp-install-serviceworker/0.1/amp-install-serviceworker.js#L381-L407

However, it seems unfortunate to have to manually add an attribute to each link that you want to prefetch. It would seem much more user friendly to automatically prefetch links that are visible in the current viewport, which is what Quicklink enables. There's actually an open issue to add a Quicklink component to AMP: ampproject/amphtml#19905. The automatic prefetching of links in the current viewport is a sensible default which could be overridden with configuration.

I don't know the full history of link-prefetch module here, but to me it seems like it's a better fit for an AMP component rather than the service worker. Thoughts?

• Webpack for target of worker does not respect public path. PR for the same here: webpack/webpack#8562

• For now we can add a babel transform/webpack plugin to do the same for us

Hi amp-sw community, I've noticed that the current logic of amp-sw might cause a FCP regression on AMP sites using HTTP caching on HTML documents but also using amp-sw "as is".

Below a screenshot for amp.dev showing that page refresh requests seems treated as Network First, always downloading the HTML document from Network.

If this is triggered by the enablement of an offline page I would suggest to switch to a CacheFirst strategy instead, maybe with a customisable max-age as safe lock or just relying on the SW versioning for caching.

FCP should improve significantly on sites using amp-sw with this strategy change.

The amp.dev certificate expired, which got reported in Safari as a service worker error:

@roshand reported a problem for amp.dev:

https://amp.dev/documentation/components/amp-analytics/?format=websites

The FetchEvent for "https://amp.dev/documentation/components/amp-analytics/?format=websites" resulted in a network error response: a redirected response was used for a request whose redirect mode is not "follow".
Promise.then (async)
(anonymous) @ amp-sw.js:1
``

inside the body of makeRequest in the AmpDocumentNetworkFirst class there is a check for request where it could be nullable but we return request anyways as the base case and don't state it in the method signature return type.

question really is, does calling makeRequest return something that is nullable from the NetworkFirst implementation in workbox?

See ampproject/amphtml#32195 for more details

In the AMP plugin for WordPress, work is underway (ampproject/amp-wp#1261) to incorporate features from AMP by Example's serviceworkerJs.js. However, there are key features in amp-sw.js which aren't implemented yet. Ideally the AMP plugin would be able to import amp-sw.js cleanly to take incorporate its functionality into the WordPress plugin. This, however, is not as simple as just adding the one-line importScripts call.

The service worker in WordPress is generated by the PWA feature plugin which is intended to be merged into WordPress core. This includes a copy of Workbox in itself which is aliased as wp.serviceWorker, with the intention that WordPress wouldn't be tied to Workbox forever.

The service worker in WordPress is designed to be extensible by themes and plugins to each include their own functionality. There is a PHP abstraction that they can use which translates PHP API calls into the equivalent Workbox calls (under the wp.serviceWorker namespace), allowing WordPress developers to continue using the preferred server-side language and WordPress then constructs the service worker JavaScript dynamically. In short, WordPress needs the amp-sw.js to be living alongside the other logic introduced by other players on a given WordPress install.

Given that WordPress already includes a copy of Workbox, it would be ideal if the AMP plugin could similarly bundle a custom build of amp-sw.js which does not have Workbox modules bundled inside of it. In other words, if the workbox modules were externalized as wp.serviceWorker:

diff --git a/webpack.config.js b/webpack.config.js
index 397f22d..a8438a5 100644
--- a/webpack.config.js
+++ b/webpack.config.js
@@ -49,6 +49,13 @@ module.exports = {
     chunkFilename: '[name].js',
     publicPath,
   },
+  externals: {
+    workbox: [ 'wp', 'serviceWorker' ],
+    'workbox-strategies': [ 'wp', 'serviceWorker', 'strategies' ],
+    'workbox-routing': [ 'wp', 'serviceWorker', 'routing' ],
+    'workbox-navigation-preload': [ 'wp', 'serviceWorker', 'navigationPreload' ],
+    'workbox-cache-expiration': [ 'wp', 'serviceWorker', 'expiration' ],
+  },
   target: "webworker",
   mode: "production",
   module: {

Given the above, the AMP plugin for WordPress would merely need to include something like the following PHP code to incorporate amp-sw.js into the service worker served by WordPress:

add_action( 'wp_front_service_worker', function( WP_Service_Worker_Scripts $service_workers ) {
	$service_workers->register( 'amp-sw', function() {
		$script  = file_get_contents( AMP__DIR__ . '/assets/js/amp-sw.js' );
		$script .= sprintf( 'AMP_SW.init( %s );', wp_json_encode( AMP_Service_Worker::get_options() ) );
		return $script;
	} );
} );

Also, the PWA plugin in WordPress already enables navigation preload, so we could exclude that functionality entirely from a custom WordPress build of amp-sw.js. Likewise, the PWA WordPress plugin also provides configuration for whether to clientsClaim and skipWaiting.

So, what could be done to facilitate amp-sw.js into WordPress in this way?

I get this exception occasionally for the amp.dev serviceworker. I'm not 100% sure how to reproduce this. Loading the page -> clearing cache in DevTools -> reloading a few times sometimes triggers it.

amp-sw.js:1 Uncaught (in promise) DOMException: Failed to execute 'transaction' on 'IDBDatabase': The database connection is closing.
    at https://cdn.ampproject.org/sw/amp-sw.js:1:8225
    at new Promise (<anonymous>)
    at r.transaction (https://cdn.ampproject.org/sw/amp-sw.js:1:8188)
    at async r._call (https://cdn.ampproject.org/sw/amp-sw.js:1:8591)
    at async r.put (https://cdn.ampproject.org/sw/amp-sw.js:1:7118)
    at async i.setTimestamp (https://cdn.ampproject.org/sw/amp-sw.js:1:9212)
    at async y.updateTimestamp (https://cdn.ampproject.org/sw/amp-sw.js:1:11953)
    at async i.cacheDidUpdate (https://cdn.ampproject.org/sw/amp-sw.js:1:14642)
    at async Object.put (https://cdn.ampproject.org/sw/amp-sw.js:1:17694)

This would not need a condition check if the denylist was always an array.

• We need purgeOnCacheEffor for asset caching module, this should be on by default but the user should be able to override it.

Resource: https://developers.google.com/web/tools/workbox/guides/storage-quota

Right now we only have core.js and optional.js as our output files.
We'd like to rename core.js to amp-sw.js and split optional.js to a single js file per module.

The SW caches a number of URL types (images, documents, etc). One that appears to be missing is <amp-list> API calls. Consider a category page on an ecomm site. It might fetch a default set of products to show on the category page, but if there is a network problem, no cache will be used to display the old set of products.

I think can assume the server will return HTTP headers saying how long the response can be cached for (including '0' for "do not cache this resource"). If a request is made when there is no network, it would be good to:

(1) Display the old page (better than displaying nothing).

(2) Indicate the network is temporarily offline / unreliable.

Not as important as the other caches, but I thought I would mention it / ask why it is not supported.

Webpack is inserting quite a bit of boilerplate into the output. Let's move to Rollup and remove it.

The offline page is currently not working for amp.dev. Here is our serviceworker:

importScripts('https://cdn.ampproject.org/sw/amp-sw.js');
AMP_SW.init({
  assetCachingOptions: [{
    regexp: /\.(png|jpg|woff2|woff|css|js)/,
    cachingStrategy: 'CACHE_FIRST',
  }],
  offlinePageOptions: {
    url: '/offline.html',
    assets: [],
  },
});

Steps to reproduce:

1. visit amp.dev
2. open DevTools and check if sw is installed and /offline.html is cached
3. Trigger offline mode in DevTools
4. Reload the page => works
5. click a link to an uncached page
   => no offline page is shown

Public without a reason.

This should be configurable.

A lot of web apps are built in paired mode where some pages are valid amp while others are not.
This library has a very clear intent of not acting on the Non AMP pages.

But if a dev thinks that our approach is good enough for the NON-amp pages as well, they should have a way to opt-in for those documents in document-caching module.

This will be a clear opt-in from the dev's side and not us doing anything un intuitive.

Unversioned js files like https://cdn.ampproject.org/v0.js are cached for 5 minutes and deployed every seven days. We keep this cache age low so that we can quickly deploy a fix if we happen to ship any.
But in AMP-service worker we serve these files with a staleWhileRevalidate strategy which means if a user lands on a buggy version, he will still get the same buggy version next time but will be fixed on the third time.

We wanted to keep this number low initially so that we don't get stuck to this situation for a long time.

But this might not be the best UX as many web apps are visited after a gap of one day and might have to download the scripts before serving to the page even if the scripts did not change.
With current defaults this can happen upto ~6 times a week for the user. So a better UX would be to the AMPs release schedule instead and let the scripts update in background.

Move from yarn to npm, since testing the release process relies on dry-run which isn't implemented in yarn.

Sine we are now widely releasing this, please polish the README and especially make the usage complete (including how to install the SW in AMP).

CC @nainar

In early design builder was a core component to be consumed by the end users, but now it is only used to create a service worker for e2e tests.

Its better to move it out of src directory.