Eliminate need for disabling navigation preload while adding navigation request caching strategy

[westonruter]

Revisits #80/#67.

At the moment, when trying to enable a navigation request caching strategy (e.g. Network-First or Stale-While-Revalidate) you also have to disable navigation preload in order to successfully get the cached pages to be served instead of the offline page:

Navigation preload was developed to improve performance of navigation requests while the service worker is not booted.

The problem came down to incorrectly using strategy.makeRequest() when it should have been using strategy.handle().

Testing

Given a theme that has the following configuration for the service worker to enable a Network-First caching strategy for navigation requests:

/*
 * Opt-in to Network-First caching strategy for navigation requests so that once you visit a page, you can go back to
 * that page even when you are offline, even when restarting the browser or reloading the page.
 */
add_filter(
	'wp_service_worker_navigation_caching_strategy',
	function() {
		return WP_Service_Worker_Caching_Routes::STRATEGY_NETWORK_FIRST;
	}
);

/*
 * Configure navigation request caching by limiting it to 50 entries and using the cache named 'pages'.
 * See https://github.com/xwp/pwa-wp/issues/176 for where something like this should be provided by default.
 */
add_filter(
	'wp_service_worker_navigation_caching_strategy_args',
	function( $args ) {
		$args['cacheName']                           = 'pages';
		$args['plugins']['expiration']['maxEntries'] = 50;
		return $args;
	}
);

Before

Preload is not being used:

🚫 Offline page always displays even though it has been cached using the navigation caching strategy.

After

Preload is being used:

✅ Cached pages are displayed.

Additionally, an admin notice is added to help guide users to correct usage given that the readme was incorrect:

Even if the user does nothing, the navigation preload will be forced to be true, though a _doing_it_wrong() will be logged. Also, Site Health will inform of the navigation preload status. When it is being erroneously disabled:

And when it is left enabled, with test passing:

Lastly, when calling \WP_Service_Workers::serve_request() it will turn off display_errors to prevent debug messages from causing JS syntax errors in the service worker; this is what is done in the WP REST API as well.

[westonruter]

@iandunn Can you test this to ensure that it eliminates the need for this WordCamp.org code:

	/*
	 * todo
	 *
	 * All of this needs to be understood deeper in order to know if it's the right way to achieve the goals
	 * of this project, and what the unintended side-effects may be.
	 *
	 * See https://developers.google.com/web/updates/2017/02/navigation-preload
	 *
	 * Should navigation preloading really be disabled? Maybe it's good to disable it since we're not using it?
	 * But then why is it enabled by default? Maybe the `pwa` plugin is using it?
	 *
	 * We need to clearly document what's going on here, and _why_.
	 *
	 * Are the chosen caching strategies and parameters appropriate in this context?
	 *
	 * Are there side-effects beyond the day-of template? If so, how should they be addressed?
	 *
	 * All of this needs to be tested to verify that it's working as intended.
	 *      What's the best way to do that? Document it here if it's not obvious.
	 */
	add_filter( 'wp_service_worker_navigation_preload', '__return_false' );

[westonruter]

This is ultimately the crux of the fix: passing event. When this is done, it allows for event.preloadResponse to be accessed:

https://github.com/GoogleChrome/workbox/blob/e33e8dcb2e207d97de07a4df9a013be341d58f7b/packages/workbox-core/src/_private/fetchWrapper.ts#L51-L63

[iandunn]

I didn't get to this today, but will try to tomorrow.

[iandunn]

Yeah, if I'm understanding it correctly, then that looks like it works to me.

When using this branch, I can remove the wp_service_worker_navigation_preload callback and still navigate to previously-visited pages while offline.

Although, I can do that on master too, even with the callback disabled. Is that expected?

[westonruter]

Did you make sure the service worker updated when you switched back to master? I would have expected the offline page to always be served instead of the cached page when navigation preload was enabled previously.

[westonruter]

I just re-checked by having PWA on master with theme containing:

add_filter(
	'wp_service_worker_navigation_caching_strategy',
	function() {
		return WP_Service_Worker_Caching_Routes::STRATEGY_NETWORK_FIRST;
	}
);
add_filter(
	'wp_service_worker_navigation_caching_strategy_args',
	function( $args ) {
		$args['cacheName']                           = 'pages';
		$args['plugins']['expiration']['maxEntries'] = 50;
		return $args;
	}
);
add_filter('wp_service_worker_navigation_preload', '__return_false');

I then opened an Incognito Window.

I then navigated to each of the nav menu items while online, and then I checked the Offline checkbox in DevTools and tried going to each of those nav menu items, and I was able to successfully go to each previously-navigated page without seeing the Offline page.

Them, while still on PWA master I removed this line:

add_filter('wp_service_worker_navigation_preload', '__return_false');

I went back Online and I reloaded the page, ensuring the updated service worker was installed. (Easy way is to close Incognito Window then open a new one.)

🚫 Then while Online and after I go to each nav menu item, if I then go Offline and try navigating to a previously-visited page, I then get my theme's offline page served to me, even though the page was cached.

✅ Then if I switch to this fix/offline-navigation-preload-enabled branch, re-open Incognito Window and again navigate to each of the nav menu items, go offline, and try going to the nav menu items again, then I can access the previously-navigated pages without having disabling navigation preload. Workbox also indicates navigation preload is enabled when I look in the console when first navigating to the site in Incognito Mode:

[westonruter]

@iandunn Any luck with reproducing the issue and fix?

[iandunn]

Haven't had time to circle back to this yet, but it's still on my list. I'll see if I can make time for it today or tomorrow.

[westonruter]

I believe this code is also unnecessary now:

https://github.com/xwp/pwa-wp/blob/d90122220378d24a2c3f376ce8453aeee47bb2b3/wp-includes/js/service-worker-navigation-routing.js#L245-L251

[westonruter]

Nevermind, that code does seem necessary.

Without the code, I see an error message in the console when Offline:

The service worker navigation preload request was cancelled before 'preloadResponse' settled. If you intend to use 'preloadResponse', use waitUntil() or respondWith() to wait for the promise to settle.

Whereas, when that code is present I see:

The latter is expected, but the former is not.

So let's leave the code there for now.

[iandunn]

Sorry for the delay, I just tested again, and I'm tracking all the way until 🚫 Then while Online...

At that point, I do get the cached versions of the nav menu items while offline, even though I'm on master and have add_filter( 'wp_service_worker_navigation_preload', '__return_false' ) commented out. I closed the Incognito window before loading the pages to make sure that the worker would be updated.

Here's some screenshots in case I'm missing something. The browser screenshots were after trying to load https://2016.misc.wordcamp.test/code-of-conduct/.

Possibly related to those TypeError: Failed to fetch entries, if I go to the Application tab and click on the Source: ?wp_service_worker=1 link, the preview in the Sources tab shows it correctly, but if instead I right-click and open it in a new tab, I get this:

This site can’t be reached
The webpage at https://2016.misc.wordcamp.test/?wp_service_worker=1 might be temporarily down or it may have moved permanently to a new web address.
ERR_FAILED

...even though curling that same URL responds with the expected script. I think that's expected, but I'm not 100% sure, so I figured I'd mention it.

[westonruter]

@iandunn Sorry for the delay. I think I see why your testing yielded different results. I believe the difference is that I was trying to do a browser reload of pages which the service worker had cached. Before the fix in this PR, if navigation preload is enabled, then doing a reload of a previously-cached page will show the offline screen.

I've created 3 new test environments to illustrate the difference:

1. ⚠️ https://not-preload-showcase-pwa.pantheonsite.io/ — Network-first navigation caching strategy, with navigation preload disabled. The same code is running on the WCEU site.
2. 🚫 https://yes-preload-showcase-pwa.pantheonsite.io/ — Network-first navigation caching strategy with navigation preload enabled. Same configuration as WCEU site, but without filtering wp_service_worker_navigation_preload to be false. (The failure here can be seen when trying to reload cached pages.)
3. ✅ https://fix-preload-showcase-pwa.pantheonsite.io/ — Same as previous, but with fix applied. Here the network-first caching strategy can be used and navigation preload can remain enabled.

Here is how to test:

1. For each site open in a new incognito window and click each top-level navigation item, followed by clicking on the Home nav menu item at the end.
2. Then open the Application tab in DevTools and go Offline.
3. Click each top-level nav menu item and see that you can successfully go to each page.
4. Still offline, navigate to another URL on the site which you haven't gone to before (e.g. July 2019 Archives) and this should show the default Offline template for each environment.
5. Still offline, go back to the homepage and try doing a browser reload (Command+R, but not Command+Shift+R as that bypasses the service worker). This will fail with an Offline screen on https://yes-preload-showcase-pwa.pantheonsite.io/ where navigation preload is enabled but the fix is not applied. On https://not-preload-showcase-pwa.pantheonsite.io/ and https://fix-preload-showcase-pwa.pantheonsite.io/ doing a reload should work fine.

Again, the missing piece here is that browser reload will not result in the Offline page when navigation preload is enabled with the fixes in this PR. Please confirm 😄

---

Each environment has this plugin active:

<?php
/**
 * Plugin Name: Network-First Navigation Caching Strategy
 * Description: With runtime caching of images, scripts, and styles.
 */

add_filter(
	'wp_service_worker_navigation_caching_strategy',
	function () {
		return WP_Service_Worker_Caching_Routes::STRATEGY_NETWORK_FIRST;
	}
);

add_filter(
	'wp_service_worker_navigation_caching_strategy_args',
	function ( $args ) {
		$args['cacheName'] = 'pages';

		$args['plugins']['expiration']['maxEntries'] = 50;

		return $args;
	}
);

add_action(
	'wp_front_service_worker',
	function( WP_Service_Worker_Scripts $scripts ) {
		// Cache scripts and styles.
		$scripts->caching_routes()->register(
			'/.*\.(?:js|css)(\?.*)?$',
			array(
				'strategy'  => WP_Service_Worker_Caching_Routes::STRATEGY_NETWORK_FIRST,
				'cacheName' => 'assets',
				'plugins'   => array(
					'expiration' => array(
						'maxEntries' => 60,
					),
				),
			)
		);

		// Cache images.
		$scripts->caching_routes()->register(
			'/wp-content/.*\.(?:png|gif|jpg|jpeg|svg|webp)(\?.*)?$',
			array(
				'strategy'  => WP_Service_Worker_Caching_Routes::STRATEGY_CACHE_FIRST,
				'cacheName' => 'images',
				'plugins'   => array(
					'expiration' => array(
						'maxEntries' => 60,
					),
				),
			)
		);
	}
);

And then only active on https://not-preload-showcase-pwa.pantheonsite.io/ is this plugin:

<?php
/**
 * Plugin Name: Disable Navigation Preload
 */

add_filter( 'wp_service_worker_navigation_preload', '__return_false' );

[westonruter]

Again, the point here is that navigation preload should always be enabled when using a navigation caching strategy because it improves network performance when navigating to a site whose service worker is not booted up (the browser doesn't have to wait for the service worker to be available).

[westonruter]

@iandunn Can you give this another look when you get a chance?

[iandunn]

Will do, sorry for the delay, I'm always juggling a lot of different things :/

[iandunn]

Hey, sorry for the delay. I was able to test this successfully now.

Part of the problem last time may have been #175 (comment).