Add ability to serve the default offline page when the user or site is offline. #48

miina · 2018-08-06T16:30:56Z

Fixes #45 .

Precache and route the offline page URL, along with its featured image, in the frontend service worker.
Serve offline page when unable to navigate to a URL due to do a server error or connection error.
Precache and route the theme's header image, background image, and custom logo (with URLs in srcset).
Precache and route all enqueued scripts and styles.
Add runtime caching with stale-while-revalidate strategy for any images and fonts inside of themes (such as referenced in the stylesheets).
Override wp.serviceWorker.routing with instance of WPRouting and frozen public API on the same way Workbox does.
Add is_offline() template conditional.

…erve-offline-template

westonruter · 2018-08-07T00:39:28Z

wp-includes/js/service-worker.js

+					handler: matchedRoute.handler
+				};
+
+				// @todo Confirm approach for conflicting routes: for example perhaps we shouldn't consider multiple matches but same strategy a conflict. Maybe we should still use the first matching route and warn about others.


Agreed. I think it's a good idea to let the first match handle the request, and then to warn about the conflicting handlers.

westonruter · 2018-08-07T02:14:52Z

wp-includes/class-wp-service-workers.php

+	 *
+	 * @return string Script.
+	 */
+	protected function configure_offline_page() {


This should ideally tie-in to a precaching API (e.g. register_precaching_for_routes) instead of having its own separate logic.

Updated the logic within dec04fb to use register_precached_routes for the precaching part.

Wondering if we should rename register_precaching_for_routes to get_precaching_for_routes_script or something similar, otherwise maybe it's too similar with register_precached_routes. Also, one is registering the route for caching and the other is generating and returning the script. Thoughts?

Yes, good idea.

Updated within a71a0cd.

westonruter · 2018-08-07T02:20:23Z

wp-includes/class-wp-service-workers.php

+			return;
+		}
+
+		if ( self::STRATEGY_PRECACHE === $strategy ) {


Instead of including precache in this method, I think it should probably be a separate method like register_precached_route. This then can be a straight wrapper for workbox.precaching.precache(): https://developers.google.com/web/tools/workbox/reference-docs/latest/workbox.precaching#.precache

westonruter · 2018-08-07T02:26:41Z

wp-includes/class-wp-service-workers.php

+	 */
+	protected function register_precaching_for_routes( $routes ) {
+
+		$routes_list = '[';


It would be better to construct an array here, like $routes_list = array(). And then for each $route do:

$routes_list[] = array( 'url' => $base_url . $route, 'revision' => $revision, );

And then at the end just do:

return sprintf( "wp.serviceWorker.precaching.precacheAndRoute( %s );\n", wp_json_encode( $routes_list ) );

westonruter · 2018-08-07T02:33:56Z

wp-includes/class-wp-service-workers.php

+		if ( isset( $args['max_entries'] ) || isset( $args['max_age'] ) ) {
+			$script .= 'args.plugins = [
+	new wp.serviceWorker.expiration.Plugin({';
+			if ( isset( $args['max_age'] ) ) {


The the max_age shouldn't be hard-coded here in $args I don't think. Instead, $args should have a plugins array like:

array( 'plugins' => array( 'expiration' => array( 'maxAgeSeconds' => 123, 'maxEntries' => 45, ), ) )

This would then allow any plugin to be referenced and passing all of the args that the plugins accept. The $args would be whatever the caching strategy options are accepted, for example: https://developers.google.com/web/tools/workbox/reference-docs/latest/workbox.strategies.CacheFirst#constructor_1

So $args should be renamed $strategy_args for clarity.

That being the case, I think the $regex should not be passed in the $args now. Perhaps there should be a separate methods like register_cached_route_pattern() and register_cached_route_url() to differentiate between the two types of args allowed?

Made the general Workbox related changes within 6c2592f in #43.

Makes sense that the plugins shouldn't be hardcoded and it should be possible to add other plugins, too. Made these changes within 6c2592f.

On regex: what do you think of just adding an additional param to the method so that if a developer would switch from between regex/url then only a param would require changing instead of the method? For example: register_cached_route( $route, $strategy, $strategy_args = array(), $is_regex = false ).
Thinking that since the only difference it makes when registering routes to self::$registered_caching_routes would be a change if is_regex is true/false for the relevant route, then maybe an additional param would be enough.

Or are you thinking that maybe it's more straightforward and better structured to have two separate methods? Thoughts?

(Added $is_regex param as a temporary change within 6c2592f to move this out of the $strategy_args, will it change it accordingly.)

I'm wary of having too many positional parameters, and if $is_regex is the 4th argument, then it is far away from the $route that it is managing. We could have such a method still but let it be protected and then the other public methods could pass this 4th arg. (Note that workbox.routing.registerRoute() has a 3rd argument, after the strategy, and it is the HTTP method to cache, which it is GET by default.) It's unfortunate that there is no native RegExp type in PHP.

If going with a 4th parameter, then it should be an $options array instead. This could contain the method and is_regex args. But then again, I suspect that few plugins would be handling non-GET requests and if they wanted to they could register custom JS directly into the SW.

Makes sense. Split the method into two within 0fd9320.

…erve-offline-template

…plugin assets on runtime.

westonruter

I'm starting work on this.

westonruter · 2018-08-13T20:26:25Z

wp-includes/class-wp-service-workers.php

+		if ( $offline_page_id ) {
+
+			// Cache relevant theme assets.
+			$this->register_cached_route( '/wp-content/.*\.(?:css|gif|png|jpg|jpeg)', self::STRATEGY_STALE_WHILE_REVALIDATE );


This will be problematic because it will store all uploaded files in the cache. We'll need to limit this to the actual theme directory URL.

westonruter · 2018-08-13T20:27:24Z

wp-includes/class-wp-service-workers.php

+	 * @return string Script.
+	 */
+	protected function configure_offline_page() {
+		$offline_page_id = (int) get_option( WP_Offline_Page::OPTION_NAME, 0 );


We should short-circuit if this is not a valid page.

westonruter · 2018-08-13T20:33:12Z

wp-includes/class-wp-service-workers.php

+	 */
+	protected function get_offline_post_image_routes( $post ) {
+		$routes = array();
+		preg_match_all( '/< *img[^>]*src *= *["\']?([^"\']*)/i', $post->post_content, $matches );


I think we need to actually extract the attachment image IDs, as we'll need to also obtain the images defined in the srcset and sizes attributes (via wp_get_attachment_image_srcset() and wp_calculate_image_sizes() respectively).

westonruter · 2018-08-13T20:34:18Z

wp-includes/class-wp-service-workers.php

+			$routes[] = array(
+				'url'      => $this->remove_url_scheme( $featured_img_url ),
+				'revision' => $post->post_modified,
+			);


Per above, we'll also need to calculate the image URLs for the srcset and sizes, as the post-thumbnail image may not actually be the size used on the offline page.

westonruter · 2018-08-13T20:35:08Z

wp-includes/js/offline-page-handling.template.js

@@ -0,0 +1,26 @@
+/* global OFFLINE_PAGE_URL */
+( () => {


My old JS habits are carrying through here. We only need to wrap in a { block since the const variables are scoped lexically. No need for IIFE.

westonruter · 2018-08-13T20:57:28Z

wp-includes/js/offline-page-handling.template.js

+( () => {
+
+	// Add custom offline / error response serving.
+	const networkFirstHandler = wp.serviceWorker.strategies.networkFirst( {


I think we need to re-use precache as the cacheName for this so that it will use what was already cached.

Nevermind. It isn't required to use cacheName: wp.serviceWorker.core.cacheNames.precache because all of the caches are being looked at for finding the offline page.

The offline page will be pre-cached.

westonruter · 2018-08-14T02:56:03Z

wp-includes/class-wp-service-workers.php

+			$this->register_precached_routes( array_merge(
+				array(
+					array(
+						'url'      => $this->remove_url_scheme( get_the_permalink( $offline_page_id ) ),


I think actually we won't need remove_url_scheme because all of the URLs should be getting served as HTTPS anyway.

* Skip looking for assets in content for now. * Remove unnecessary remove_url_scheme. * Consolidate precache URLs into single get_offline_page_precache_entries method. * Add helper get_offline_page method for getting published offline page.

* The offline page will be served from the precache so no caching will be done during navigation. * Blacklist /wp-admin/ from serving the offline page; a separate admin offline page will be needed.

westonruter

This is working well for serving the offline page. There is more work to do on the offline page and the API for precaching, but we'll continue iterating in subsequent PRs.

miina and others added 6 commits August 6, 2018 19:12

Start adding basic serving of offline page.

ce70f66

Precache and serve the page set by offline page option.

dfb8869

Wrap WPRouter definition in IIFE; format jsdoc

89fb25b

Move JS into template file

7c155e8

Use modified date instead of hashed content

ca41f10

Merge branch 'master' of https://github.com/xwp/pwa-wp into feature/s…

d0b543c

…erve-offline-template

westonruter reviewed Aug 7, 2018

View reviewed changes

westonruter mentioned this pull request Aug 7, 2018

Workbox API Prototype #43

Merged

miina added 4 commits August 7, 2018 20:05

Use Workbox API to precache offline page.

dec04fb

Remove redundand code.

e99bc51

Merge Workbox API PR & resolve conflicts.

73b4d76

Fix phpcs.

081b7f5

westonruter mentioned this pull request Aug 9, 2018

Idea: Add support for custom theme template file to be used for offline page #50

Closed

westonruter and others added 2 commits August 10, 2018 13:02

Merge branch 'master' of https://github.com/xwp/pwa-wp into feature/s…

ffd615c

…erve-offline-template

Add precaching for images within offline page content. Cache theme / …

7edcbe5

…plugin assets on runtime.

miina changed the title ~~[WIP] Add ability to serve the default offline page when the user or site is offline.~~ Add ability to serve the default offline page when the user or site is offline. Aug 13, 2018

miina added 2 commits August 13, 2018 17:32

Add is_offline() function.

aaa80d7

Precache the featured image of the offline page, if found.

d2d0795

westonruter reviewed Aug 13, 2018

View reviewed changes

westonruter self-assigned this Aug 13, 2018

westonruter added 2 commits August 13, 2018 17:25

Prevent storing navigated pages in the runtime cache

8264dae

The offline page will be pre-cached.

Limit runtime cache to parent/child theme assets

71009ec

westonruter reviewed Aug 14, 2018

View reviewed changes

westonruter added 7 commits August 13, 2018 20:35

Cache offline page featured image srcset URLs

547d96b

* Skip looking for assets in content for now. * Remove unnecessary remove_url_scheme. * Consolidate precache URLs into single get_offline_page_precache_entries method. * Add helper get_offline_page method for getting published offline page.

Use NavigationRoute without networkFirst strategy

7bc3624

* The offline page will be served from the precache so no caching will be done during navigation. * Blacklist /wp-admin/ from serving the offline page; a separate admin offline page will be needed.

Override wp.serviceWorker.routing with WPRouter instance

198536b

Precache the custom logo, custom header, and custom background

a3db2db

Use WP_Service_Workers::QUERY_VAR constant

c558afe

Limit addition of offline page caching to front service worker

5b8a0b6

Add WP_Service_Workers::get_current_scope() helper method

d0a4937

westonruter added 2 commits August 14, 2018 21:43

Add precaching of all enqueued scripts and styles

47f3c81

Limit runtime caching to images and fonts in themes

2709b08

westonruter approved these changes Aug 15, 2018

View reviewed changes

westonruter merged commit 9a10dc7 into master Aug 15, 2018

This was referenced Aug 15, 2018

Implement precaching #25

Closed

Serve static offline/500 template as opposed to using specially-designated page #52

Closed

Remove user-specified offline page; add static theme-overridable offline/500 templates #53

Merged

westonruter added the service-workers label Aug 28, 2018

westonruter added this to the 0.2 milestone Apr 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to serve the default offline page when the user or site is offline. #48

Add ability to serve the default offline page when the user or site is offline. #48

miina commented Aug 6, 2018 •

edited by westonruter

Loading

westonruter Aug 7, 2018

westonruter Aug 7, 2018

miina Aug 7, 2018

westonruter Aug 7, 2018

miina Aug 8, 2018

westonruter Aug 7, 2018

westonruter Aug 7, 2018

westonruter Aug 7, 2018

westonruter Aug 7, 2018

miina Aug 7, 2018

westonruter Aug 7, 2018

miina Aug 8, 2018

westonruter left a comment

westonruter Aug 13, 2018

westonruter Aug 13, 2018

westonruter Aug 13, 2018

westonruter Aug 13, 2018

westonruter Aug 13, 2018

westonruter Aug 13, 2018

westonruter Aug 14, 2018

westonruter Aug 14, 2018

westonruter left a comment

Add ability to serve the default offline page when the user or site is offline. #48

Add ability to serve the default offline page when the user or site is offline. #48

Conversation

miina commented Aug 6, 2018 • edited by westonruter 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

westonruter 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

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

westonruter left a comment

Choose a reason for hiding this comment

miina commented Aug 6, 2018 •

edited by westonruter

Loading