Add navigation caching strategy #89

westonruter · 2018-10-16T22:54:48Z

With the changes here, a theme or plugin can opt-in to the staleWhileRevalidate caching strategy for navigation routes via code like:

add_filter( 'wp_service_worker_navigation_preload', '__return_false' );

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

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

	$args['plugins']['expiration']['maxEntries'] = 50;
	return $args;
} );

wp-includes/js/service-worker-navigation-routing.js

…nent for reuse

westonruter · 2018-10-20T06:03:46Z

Example for how to utilize: https://gist.github.com/westonruter/f848013108672568be6dcde153f9ca37

westonruter · 2018-10-20T06:25:58Z

wp-includes/components/class-wp-service-worker-navigation-routing-component.php

+			 *
+			 * @param string $caching_strategy Caching strategy to use for frontend navigation requests.
+			 */
+			$caching_strategy = apply_filters( 'wp_service_worker_navigation_caching_strategy', WP_Service_Worker_Caching_Routes::STRATEGY_NETWORK_ONLY );


It would seem ideal to not have to resort to filters here, but rather to be able to do this in the wp_front_service_worker action:

$scripts->navigation_routing->set_caching_strategy( WP_Service_Worker_Caching_Routes::STALE_WHILE_REVALIDATE, array( /* ...args... */) );

This would be good to look at as part of iterating on the PHP architecture. /cc @felixarntz

Haha. It seems my failed attempts last night to post this comment on GitHub when the site was down were not failures after all.

In short, the idea is to let there be a proper method for setting the navigation caching strategy rather than just resorting to a filter.

So instead of doing:

$caching_strategy = apply_filters( 'wp_service_worker_navigation_caching_strategy', WP_Service_Worker_Caching_Routes::STRATEGY_NETWORK_ONLY );

There could be an explicit call from plugin code that runs in the wp_front_service_worker action, like so:

add_action( 'wp_front_service_worker', function( $scripts ) { $scripts->navigation_routing->set_caching_strategy( WP_Service_Worker_Caching_Routes::STRATEGY_NETWORK_ONLY ); } );

This could be done for other extension points as well, such as managing navigation preload.

Maybe it would be more elegant way of managing these things, or maybe it is unnecessary and not as WordPressy.

I think the filter approach is good for now. The templates can be modified through filters as well, and IMO it makes sense to continue along with it. This component is rather specific and not as generic as the ones for caching and precaching routes, which both furthermore have in common that they use registries. Last but not least, this component is hooked in to wp_*_service_worker at a very early priority (as it should), so relying on these same actions could result in problems due to hook priorities. I'm +1 for the filters.

One question I have though is, why do we only expose the filter for the frontend? Wouldn't it make sense in the admin too?

One question I have though is, why do we only expose the filter for the frontend? Wouldn't it make sense in the admin too?

Mainly because I wanted to keep the admin out of scope for now. Currently the offline and 500 error templates in the admin are hard-coded and cannot be customized. I was thinking that WordPress core would be responsible for managing how the admin would work as a PWA and that it wouldn't be in the realm of plugin/theme authors primarily to manage fundamentally.

felixarntz

This looks ALMOST good to go for me, aside from a few minor fixes and clarifications.

wp-includes/components/class-wp-service-worker-caching-routes-component.php

felixarntz · 2018-10-23T21:54:08Z

wp-includes/components/class-wp-service-worker-caching-routes-component.php

+	 * @param array $strategy_args Strategy args.
+	 * @return string JS IIFE which returns object for passing to registerRoute.
+	 */
+	public static function prepare_strategy_args_for_js_export( $strategy_args ) {


+1 for separate method.

felixarntz · 2018-10-23T21:54:31Z

wp-includes/components/class-wp-service-worker-caching-routes-component.php

@@ -156,7 +165,7 @@ public function get_script() {
 	 * @param array $matches Matches.
 	 * @return string Replaced string.
 	 */
-	protected function convert_snake_case_to_camel_case_callback( $matches ) {
+	protected static function convert_snake_case_to_camel_case_callback( $matches ) {


Why do we need this method to be static?

Actually, I think I neglected to make it public instead of protected. I made this change for the sake of PHP 5.3 which isn't able to use a protected class method in a preg_replace_callback call, if I recall correctly.

I'm fairly certain it does support protected/private callbacks, even in 5.2. The only things that don't are WP hooks :)

Would be great if you could double-check.

See http://php.net/manual/de/function.preg-replace-callback.php#110353

Ah, I remember the reason. The protected issue is what I had about the replace_exported_variable method actually.

The actual reason why I made this method static was because it is called from prepare_strategy_args_for_js_export which is a static method. This method is called in WP_Service_Worker_Caching_Routes_Component::get_script() and in \WP_Service_Worker_Navigation_Routing_Component::serve().

I'd prefer to not rely on static methods unless absolutely necessary, or at least clarify its nature and usage. For this specific case, to me the prepare_strategy_args_for_js_export() appears to be a utility method. If it was just used in one class, it could remain non-static and protected/private. Since it is however needed by another class too, I suggest to either outsource it as a static method into a utility class, or just as a plan utility function. Having it in WP_Service_Worker_Caching_Routes_Component although it is also needed by WP_Service_Worker_Navigation_Routing_Component feels confusing.

I realized that this method should have been put into the WP_Service_Worker_Caching_Routes class.

wp-includes/components/class-wp-service-worker-navigation-routing-component.php

felixarntz · 2018-10-23T22:11:56Z

wp-includes/components/class-wp-service-worker-navigation-routing-component.php

+			 *
+			 * @param string $caching_strategy Caching strategy to use for frontend navigation requests.
+			 */
+			$caching_strategy = apply_filters( 'wp_service_worker_navigation_caching_strategy', WP_Service_Worker_Caching_Routes::STRATEGY_NETWORK_ONLY );


I think the filter approach is good for now. The templates can be modified through filters as well, and IMO it makes sense to continue along with it. This component is rather specific and not as generic as the ones for caching and precaching routes, which both furthermore have in common that they use registries. Last but not least, this component is hooked in to wp_*_service_worker at a very early priority (as it should), so relying on these same actions could result in problems due to hook priorities. I'm +1 for the filters.

One question I have though is, why do we only expose the filter for the frontend? Wouldn't it make sense in the admin too?

felixarntz · 2018-10-23T22:12:45Z

wp-includes/components/class-wp-service-worker-navigation-routing-component.php

-		return str_replace(
-			array_keys( $this->replacements ),
-			array_values( $this->replacements ),
+		return preg_replace_callback(


Good catch fixing this for the new placeholders!

westonruter · 2018-10-24T23:43:50Z

Let's merge this after #92, since it will create conflicts for that PR.

…ation-caching-strategy

…Caching_Routes class

felixarntz · 2018-11-06T16:16:30Z

wp-includes/components/class-wp-service-worker-caching-routes.php

+	 * @param array $strategy_args Strategy args.
+	 * @return string JS IIFE which returns object for passing to registerRoute.
+	 */
+	public static function prepare_strategy_args_for_js_export( $strategy_args ) {


Ah! Having this here makes a lot more sense to me, and then having the method being static for outside access appears somewhat okay too - I'm sure we can find a better architecture for this, but I'm okay with proceeding as is for now.

westonruter force-pushed the add/navigation-caching-strategy branch from 0f88769 to a958209 Compare October 18, 2018 20:51

westonruter commented Oct 18, 2018

View reviewed changes

wp-includes/js/service-worker-navigation-routing.js Outdated Show resolved Hide resolved

westonruter force-pushed the add/navigation-caching-strategy branch from a958209 to 0b81aaf Compare October 19, 2018 05:35

westonruter added 2 commits October 19, 2018 22:57

Extract JS serializer logic in WP_Service_Worker_Caching_Routes_Compo…

b8d3b5c

…nent for reuse

Allow navigation route to use caching strategies

d6d0cf4

westonruter force-pushed the add/navigation-caching-strategy branch from 0b81aaf to d6d0cf4 Compare October 20, 2018 05:58

westonruter commented Oct 20, 2018

View reviewed changes

westonruter changed the title ~~[WIP] Add navigation caching strategy~~ Add navigation caching strategy Oct 22, 2018

westonruter changed the title ~~Add navigation caching strategy~~ Add customizable navigation caching strategy Oct 22, 2018

westonruter changed the title ~~Add customizable navigation caching strategy~~ Add navigation caching strategy Oct 22, 2018

westonruter requested a review from amedina October 22, 2018 23:58

felixarntz requested changes Oct 23, 2018

View reviewed changes

Note the unrecognized plugin's name in the notice

d7bde46

westonruter added 2 commits November 6, 2018 07:12

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

e6ce85d

…ation-caching-strategy

Move prepare_strategy_args_for_js_export method to WP_Service_Worker_…

71aa816

…Caching_Routes class

felixarntz approved these changes Nov 6, 2018

View reviewed changes

westonruter merged commit 2ff7ac4 into master Nov 6, 2018

westonruter deleted the add/navigation-caching-strategy branch November 6, 2018 16:27

westonruter added the service-workers label Feb 25, 2019

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 navigation caching strategy #89

Add navigation caching strategy #89

westonruter commented Oct 16, 2018 •

edited

Loading

westonruter commented Oct 20, 2018

westonruter Oct 20, 2018 •

edited

Loading

westonruter Oct 22, 2018

westonruter Oct 22, 2018

westonruter Oct 22, 2018

westonruter Oct 22, 2018

westonruter Oct 22, 2018

westonruter Oct 22, 2018

westonruter Oct 23, 2018

felixarntz Oct 23, 2018

westonruter Oct 23, 2018

felixarntz left a comment •

edited

Loading

felixarntz Oct 23, 2018

felixarntz Oct 23, 2018

westonruter Oct 23, 2018

felixarntz Oct 23, 2018 •

edited

Loading

felixarntz Oct 23, 2018

westonruter Oct 24, 2018

felixarntz Oct 25, 2018 •

edited

Loading

westonruter Nov 6, 2018

felixarntz Oct 23, 2018

felixarntz Oct 23, 2018

westonruter commented Oct 24, 2018

felixarntz Nov 6, 2018

Add navigation caching strategy #89

Add navigation caching strategy #89

Conversation

westonruter commented Oct 16, 2018 • edited Loading

westonruter commented Oct 20, 2018

westonruter Oct 20, 2018 • edited 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

felixarntz left a comment • edited 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

felixarntz Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz Oct 25, 2018 • edited 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

westonruter commented Oct 24, 2018

Choose a reason for hiding this comment

westonruter commented Oct 16, 2018 •

edited

Loading

westonruter Oct 20, 2018 •

edited

Loading

felixarntz left a comment •

edited

Loading

felixarntz Oct 23, 2018 •

edited

Loading

felixarntz Oct 25, 2018 •

edited

Loading