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

Install service worker to cache AMP CDN assets, cache Google Fonts, and cache images (following AMP by Example) #1261

Merged
merged 36 commits into from
Apr 3, 2019

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jul 8, 2018

This integration can be seen live on https://amp-wp.org/

Changes:

  • In AMP responses, prevents outputting the script for installing the service worker as output by PWA-WP and instead outputs an amp-install-service-worker tag instead.
  • Add an endpoint specifically for handling the iframe-based service worker installation from AMP Cache.
  • Implement remaining aspects of @sebastianbenz's minimal service worker for websites built with AMP which aren't already implemented in the PWA plugin, namely:
    • cdn_script_caching: The runtime precaching of AMP runtime and key component scripts; using stale-while-revalidate for those and other CDN assets that are loaded at runtime.
    • google_fonts_caching: Caching of Google Fonts, unless the PWA plugin's integration isn't also running. Disabled by default.
    • image_caching: Cache-first strategy for images. Disabled by default.

To enable all integrations, add to your theme:

add_theme_support( 'amp', [
	'service_worker' => [
		'cdn_script_caching'   => true,
		'google_fonts_caching' => true,
		'image_caching'        => true,
	],
] );

To disable all integrations, you can also do:

add_theme_support( 'amp', [
	'service_worker' => false,
] );

With this PR and the PWA-WP feature plugin, running Twenty Sixteen in my development environment yields these Lighthouse results:

image

Todo

  • Add tests
  • Implement caching with Workbox
  • “SWR still needs some reasonable expiry (say 8 or 30 days) and https://fonts.gstatic.com should probably use cacheFirst instead of SWR.” (@cramforce tweet)
  • “consider using networkFirst for documents. This might be controversial, but it would be amazing. Trying that for AMP only might be a good first step to rolling it out to the rest of WP.” (@cramforce tweet) (We can address this with integration of amp-sw.)

Fixes #1210

Depends on:

Build for testing: amp.zip (v1.1-alpha-20190311T031056Z-a1cb0cdc)

includes/class-amp-service-workers.php Outdated Show resolved Hide resolved
includes/class-amp-service-workers.php Outdated Show resolved Hide resolved
includes/class-amp-service-workers.php Outdated Show resolved Hide resolved
includes/class-amp-service-workers.php Outdated Show resolved Hide resolved
@westonruter westonruter removed this from the v1.0 milestone Jul 25, 2018
@westonruter
Copy link
Member Author

I'm removing this from 1.0. It depends on the PWA plugin and/or core support, so this is something that should land later when WP support for service workers are more mature.

@westonruter
Copy link
Member Author

Refer also to this service worker from @sebastianbenz that implements all of the best practices: https://gist.github.com/sebastianbenz/1d449dee039202d8b7464f1131eae449

His service worker relies on Workbox so we should use this as a use case for GoogleChromeLabs/pwa-wp#5. Note that his service worker also includes support for offline page which is being worked on in GoogleChromeLabs/pwa-wp#23

@westonruter
Copy link
Member Author

Important note from @sebastianbenz in regards to whether a service worker should be used if a site is not a single-page app:

If none of these patterns are used and it’s not possible to cache the whole site (which only is reasonable for very small sites), a serviceworker might have a negative performance impact. The best thing in this case is to not use a serviceworker.

In this case, if the service worker is merely being used to cache AMP CDN assets then the service worker should only be used during development (when WP_DEBUG is enabled).

@postphotos
Copy link
Contributor

This is fantastic, @westonruter! Those Lighthouse scores are quite impressive. Looking forward to seeing this merge in as PWA advances forward in development.

…ce-worker

* 'develop' of github.com:ampproject/amp-wp: (52 commits)
  Add tests for amp-twitter in STAMP layer and changes to amp-brid-player
  Update allowed tags/attributes from spec in amphtml 1903262220080
  Update dependency eslint to v5.16.0
  Remove stray semicolon in AMP_Analytics_Options_Submenu
  Update install-wp-tests.sh
  Fix tests when Gutenberg is active in WP>=5.0
  Update dependency xwp/wp-dev-lib to v1.1.0
  Use render_block filter instead of overridding the render_callback
  Make archives and categories block overrides DRY
  Ensure initial clean state for test_register_and_unregister_embed
  Simplify overriding of render_callback
  Install Gutenberg even in WordPress 5.x
  Add tests for actual categories and archives output
  Add support for Archives widget when displaying as dropdown
  Add test for amp_normalized_dimension_extractor_image_url filter
  Prevent tree-shaking class names for amp-geo when component on page
  Update dependency grunt to v1.0.4
  Make IDs unique in cloned elements for sticky nav menu
  Update dependency @wordpress/i18n to v3.3.0
  Add tests for amp-base-carousel, amp-smartlinks, and new scale/translate amp-story attrs
  ...
@westonruter westonruter added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Apr 2, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Apr 2, 2019
@westonruter
Copy link
Member Author

CLA bot is complaining about felixarntz@users.noreply.github.com. So we'll just ignore that until merging, at which time we'll re-flip the label.

*
* @param WP_Service_Worker_Scripts $service_workers Service workers.
*/
public static function add_google_fonts_caching( $service_workers ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to have the check for the correct class as well, like the above (without _doing_it_wrong()).

Copy link
Member Author

Choose a reason for hiding this comment

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

Not like this?

diff --git a/includes/class-amp-service-worker.php b/includes/class-amp-service-worker.php
index 37dbc0e1..4de47a37 100644
--- a/includes/class-amp-service-worker.php
+++ b/includes/class-amp-service-worker.php
@@ -157,6 +157,10 @@ class AMP_Service_Worker {
 	 * @param WP_Service_Worker_Scripts $service_workers Service workers.
 	 */
 	public static function add_google_fonts_caching( $service_workers ) {
+		if ( ! ( $service_workers instanceof WP_Service_Worker_Scripts ) ) {
+			_doing_it_wrong( __METHOD__, esc_html__( 'Expected argument to be WP_Service_Worker_Scripts.', 'amp' ), '1.1' );
+			return;
+		}
 
 		// The PWA plugin also automatically adds runtime caching for Google Fonts when WP_SERVICE_WORKER_INTEGRATIONS_ENABLED is set.
 		if ( class_exists( 'WP_Service_Worker_Fonts_Integration' ) ) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

See the other comments above.

Copy link
Collaborator

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Resolution looks good to me now!

…ce-worker

* 'develop' of github.com:ampproject/amp-wp:
  Account for element attributes when determining if a parent is empty and can be removed
  Fix tests after always including Gutenberg
@westonruter westonruter added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Apr 3, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@westonruter westonruter modified the milestones: v1.2, v1.1 Apr 3, 2019
@westonruter westonruter changed the title Install service worker for AMP pages to cache AMP CDN assets Install service worker to cache AMP CDN assets, cache Google Fonts, and cache images (following AMP by Example) Apr 3, 2019
@westonruter westonruter merged commit 2d941f9 into develop Apr 3, 2019
@westonruter westonruter deleted the add/service-worker branch April 3, 2019 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA Integration: PWA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use service worker to cache AMP component scripts and other assets
6 participants