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

Propose a Workbox solution for WordPress #5

Closed
postphotos opened this Issue May 24, 2018 · 16 comments

Comments

Projects
8 participants
@postphotos
Copy link

postphotos commented May 24, 2018

As a contributor to this project, after I've understood and and documented how the Workbox APIs work, I should propose an architectural plan using Workbox that will allow plugins, themes and site owners alike to share PWA component features in a single service worker implementation without conflict.

Related #1. #2.

@postphotos postphotos added this to To do in v0.2 May 24, 2018

@kevincoleman kevincoleman added Sprint 3 and removed Sprint 2 labels Jun 19, 2018

@miina

This comment has been minimized.

Copy link
Contributor

miina commented Jun 27, 2018

Thinking that maybe with implementing Workbox registering an actual JS file with SW logic wouldn't be required for plugins using the API, and we could just add PHP functions for Workbox functionalities and then concatenate the relevant Workbox code together into dynamically created SW logic per scope, as it is now in the API Prototype within #14.

For example there’s a network first Workbox caching strategy which can be used for specific routes (workbox.strategies.networkFirst()). Instead of creating and registering a separate SW script file including the logic maybe with Workbox just a method something like wp_add_sw_network_first_strategy( $route, $args ) could be used and then the feature plugin would add the relevant code to the SW logic (based on the current API prototype generated at "/?wp_service_workers=$scope")

Then for example the assets of AMP Plugin (this issue) could also just be registered by using a helper method (e.g. wp_sw_precache_assets( ... )).

This would also make it possible to manage potential conflicts.

Thoughts?
cc @westonruter @kienstra

@kienstra

This comment has been minimized.

Copy link
Contributor

kienstra commented Jun 28, 2018

Workbox Strategy

Hi @miina,
Good idea to have a PHP function to generate code for the Workbox API. I think that'd be useful for some cases, like the wp_add_sw_network_first_strategy( $route, $args ) and wp_sw_precache_assets() functions you mentioned.

Is there a plan for a /wp-admin UI to show possible conflicts? Like if one plugin uses workbox.strategies.cacheFirst on a route, while another uses workbox.strategies.StaleWhileRevalidate?

Otherwise, I'm not sure how useful it would be to create a full API for the Workbox API. In the example conflict above, it looks like one just overrides the earlier one. There's no console error. Though maybe we would consider a priority argument to our API, like in add_action().

The service worker API in #14 already outputs service worker JS files that plugins or themes register via wp_register_service_worker().

If we're not going to have a /wp-admin UI to show conflicts, it seems like plugins could just include any Workbox calls in their service worker JS file that your API above registers.

Having a full API to the Workbox API seems like a lot. Developers would have to learn a new API, and we'd have to maintain documentation.

Of course, I could have overlooked some benefits of this API.

@miina

This comment has been minimized.

Copy link
Contributor

miina commented Jun 28, 2018

@kienstra Thanks for looking into this.

True that there it wouldn't make sense to create a full API for the full Workbox API.

If we're not going to have a /wp-admin UI to show conflicts, it seems like plugins could just include any Workbox calls in their service worker JS file that your API above registers.

True that if we're not handling the conflicts then there is no strong reason to create an additional layer of methods on Workbox which would just add the scripts together. The main benefit of the added API would be being able to handle the conflicts. As far as I understood from conversations and from the AC then handling the conflicts would be needed. Not sure though how would the UI look like.

Maybe adding the caching strategy would actually need just one method at first, something like:
wp_register_service_worker_caching_strategy( $route, $strategy, $args ) where $strategy would determine if it would be cacheFirst, networkFirst, StaleWhilRevalidate, or any other existing strategy.

This way it could be possible to process the routes and detect conflicts. For example if two routes overlap with the same strategy there's potentially no hard conflict (although it could be with expiration time, etc.) but if the strategy is different then a warning should be shown. Route in Workbox registerRoute is regex pattern, wondering if comparing regex patterns to find conflicts would be straightforward or if $route registered via the SW API should use some other format?

How should we handle the conflicting routes, should we still add these and just show a notice about the conflicts? Maybe we could try to combine the routes, for example if one route is for the whole site and one for a specific page then add the broader route first and then the specific route later.

Keeping the wp_register_service_worker() as it is at this moment in the API prototype would mean that anyone could still add their custom Workbox / caching logic to service worker as well, potentially causing undetected conflicts, maybe the logic added via the API layer (e.g. by using wp_register_service_worker_caching_strategy() ) on top of Workbox API would get priority and be added to the SW latest.

It's also possible to register custom handler method for the route within Workbox instead of using already existing cache strategies. Maybe we should just ignore the handler and report a conflict in all cases if the route matches.

Thoughts?

@kienstra

This comment has been minimized.

Copy link
Contributor

kienstra commented Jun 29, 2018

Great Ideas
Possible UI In Console

Hi @miina,

Maybe adding the caching strategy would actually need just one method at first, something like:
wp_register_service_worker_caching_strategy( $route, $strategy, $args ) where $strategy would determine if it would be cacheFirst, networkFirst, StaleWhilRevalidate, or any other existing strategy.

That's a good idea. It sounds good as a PHP API, and could use your service worker API prototype.

How should we handle the conflicting routes, should we still add these and just show a notice about the conflicts?

Good point. Comparing regexes for possible matches will probably be complex.

What do you think about this prototype for notices of conflicts:

proposed-display-two-routes-match

Pros

  • Uses the Native Workbox console logger API.
  • This is easier to develop than a /wp-admin UI. It's working for me locally, though it'll need refactoring.
  • This probably won't require much change in how developers use Workbox, and won't require much documentation from us.

Cons

  • There's no unified UI, like in the AMP plugin's validator.
  • There's no way to resolve the conflicting routes.
  • These messages would probably be useless to non-developers, without much more context.

Keeping the wp_register_service_worker() as it is at this moment in the API prototype would mean that anyone could still add their custom Workbox / caching logic to service worker as well, potentially causing undetected conflicts, maybe the logic added via the API layer (e.g. by using wp_register_service_worker_caching_strategy() ) on top of Workbox API would get priority and be added to the SW latest.

That's a good idea to give priority to the potential workbox calls from the API.

Your ideas for the API are really good. As an alternative, what do you think about simply allowing users to use the Workbox JavaScript API, as they do now?

I think unless we have a good /wp-admin UI to see and maybe resolve route conflicts, it might be simpler to use JS.

@postphotos

This comment has been minimized.

Copy link
Author

postphotos commented Jun 29, 2018

@miina and @kienstra, thank you for the great discussion here!

Maybe no UI for this stage, but we should definitely consider it.

I think, for MVP's sake here, we can avoid a UI inside /wp-admin/ but this would be the preferred WordPress-y way.

But, that being said, there's not a single mention of the wp-api, for example, in the current UI today, nor a single interface beyond the command line for wp-cli, which is perhaps inherent for what the tools are. I am aware of one tool that does something of the sort for wp-api. I think, even if service workers are intended for experienced developers today, we should be trying to reduce friction in setting things up and providing more context where possible.

@kevincoleman

This comment has been minimized.

Copy link

kevincoleman commented Jul 3, 2018

Thanks for digging into this, @kienstra and @miina.

@westonruter Would you mind taking a look at this to see what you think might be the best course of action?

@westonruter

This comment has been minimized.

Copy link
Collaborator

westonruter commented Jul 5, 2018

Great discussion so far. As we go beyond the foundational prerequisite of being able to register a script to be included in a service worker (#7), these are the higher-level abstractions that we should be moving toward in order to prevent theme and plugin developers from being to write (and rewrite) fetch handlers from scratch. And figuring out how to incorporate Workbox into WordPress (in a WordPressy way) seems like a great way to do that.

We should get feedback as early as possible on the ideas here from the Workbox devs and the Chrome team to get their insights and direction on the design. /cc @amedina

In terms of detecting conflicts, indeed it is not possible statically (such as when calling workbox.routing.registerRoute()) to determine if two routes' regular expressions will conflict. I think it will have to be done at runtime. I think this may require a change to Workbox itself. For instance, the Workbox docs say:

The order of the Route registration is important. ¶ If multiple Routes are registered that could handle a request, the Route that is registered first will be used to respond to the Request.

For the purposes of multiple 3rd-parties each registering their own routes, I think this logic needs to change (or be configurable) so that the router iterates over all registered routes, continuing to count matches even after the first one. If multiple match (excluding the default handler), then workbox could issue a warning.

@amedina

This comment has been minimized.

Copy link
Collaborator

amedina commented Jul 6, 2018

@jeffposnick, @gauntface, @addyosmani, @igrigorik, @pbakaus The work in this issue (and in this repo) is related to the integration of Service Workers into WordPress, which entails leveraging the capabilities of WorkBox as part of a WordPress-y solution for enabling Service Workers in WordPress. FYI/PTAL.

@amedina

This comment has been minimized.

Copy link
Collaborator

amedina commented Jul 6, 2018

Related to the earlier parts of this discussion: it may make sense to handle conflicts at the WordPress-level since the SW integration will potentially handle requests from different plugins and the theme. If we follow only the WorkBox semantics of overriding old routes with new one, sites make misbehave without notice upon installation of a new plugin that makes use of the SW API. WDYT?

@amedina

This comment has been minimized.

Copy link
Collaborator

amedina commented Jul 6, 2018

Related to the issue of providing wrapper functions for the WorkBox API: it may make sense to do that depending on the abstraction layer we would like to provide. For certain segments of the developer/site-owners space, it would probably be better not to deal with the lowest-level details of WorkBox. Thoughts?

@jeffposnick

This comment has been minimized.

Copy link

jeffposnick commented Jul 6, 2018

Hello from the Workbox team! There's a lot to unpack in this thread, so let me break up my initial reply into a few different sections. (I'm going to then make a follow-up post with some questions about handling navigations and the ultimate goal for the proposed service worker.)

Workbox's Routing

Workbox provides a routing solution as part of the workbox-routing module. This router takes care of registering a fetch handler for you, allows you to specify one or more routes, and then applies a given response strategy to the first route that matches. Using workbox-routing is optional, and is not a prerequisite for using the caching strategies that Workbox provides in separate modules (workbox-precaching and workbox-strategies).

If you decided that the Workbox router's default behavior of using a first-route-registered-wins isn't desirable, you can either implement your own fetch handlers which called the caching strategies directly (the runtime caching ones each expose a makeRequest() method that returns a Promise<Response>), or write your own custom router that adheres to the general interface of the default router. You could, for instance, use a CustomRouter class which extends Router but overrides the _findHandlerAndParams() method, which is what is responsible for picking out the matching route:

https://github.com/GoogleChrome/workbox/blob/22b855df190e7272ed13f20425a057f089e00314/packages/workbox-routing/Router.mjs#L173-L207

That custom method could then throw if multiple routes are discovered, or come up with its own way of reconciling them.

Declarative Routing via Configuration Files

There's already precedent for allowing developers to specify their Workbox routing + handler configuration via a configuration format, which is then translated into actual executable Workbox statements as part of a build process—we call this the generateSW flow. The format looks roughly like this:

runtimeCaching: [{
  urlPattern: /.jpg$/,
  handler: 'cacheFirst',
}]

and the node code that translates that at build time lives here. I'm not sure if it makes sense for you to repurpose that format, or if it's sufficient for your needs, but that's the precedent.

Precaching

Something I've not seen mentioned yet has to do with precaching—here's an explainer about how Workbox handles precaching. Precaching gives you the advantage of ensuring that, whenever your service worker is installed, a core set of URLs will be cached and kept up to date, even if they correspond to assets that haven't been used yet. (In other words, you get to cache important things in advance.)

This differs from using routing + caching strategies to implement runtime caching, in which assets and HTML is only cached after a user visits a page that uses them for the first time.

Precaching offers a number of advantages, both in terms of "priming" the cache, as well as efficient updates (a network request is only required when something in the precache manifest actually changes). The downside of precaching is that it requires some integration with a build process, as it Workbox needs to create hash "fingerprints" of local assets on the filesystem in order to create its precache manifest. That might not be viable in a generic Wordpress solution, but I wanted to throw it out there if there are hooks into a local build process that could be taken advantage of.

Workbox's Cache Management

I don't want to suggest that using Workbox is the only viable approach when it comes to powering your service worker, but I do want to point out that we've spent a lot of time thinking about the cache management story, including logic for intelligently updating a manifest of precached assets, automatically expiring entries that have been cached at runtime, as well as recovering from failure scenarios in which the user's device runs out of storage space.

Some of the things you need to think about when it comes to caching and storage quotas are outlined in this "Understanding Storage Quota" article. I'd recommend reading through it and coming up with at least some plan to handle those failure scenarios, whether it's using Workbox or something that you build yourself. We've seen from real-world deployments that quota issues are very easy to bump into unless you go out of your way to avoid them.

@jeffposnick

This comment has been minimized.

Copy link

jeffposnick commented Jul 6, 2018

I wanted to break this out into its own response. It's less about Workbox and more about confirming what the goals are for your service worker, and what's achievable with a given HTML architecture.

Handling navigations via an App Shell

Based on #12 and #22, it sounds like you're leaning towards using an App Shell.

If you're going to go with that approach, then I'd recommend using an approach that precached your shell.html along with any common subresources, like the critical styles and images used unconditionally within the shell.

You'd then need to make sure that your App Shell's cached HTML was used within your service worker to satisfy navigation requests (or potentially just a subset of those requests, if you want to avoid using the shell for subsections of a given site that need a different structure).

workbox.routing.registerNavigationRoute() can help with this, if you decide to go with Workbox.

Alternatives to the App Shell

While the App Shell model has the most real-world usage amongst PWAs right now, there are some other approaches that might be worth considering if you want to avoid the "single page app" model.

I talked about one approach at Google I/O this year, and wrote up the material into a blog post. (See also some earlier explorations on my personal blog about the same concepts.)

The main challenge with these approaches is that they work best if you could replicate the logic used on the backend server within the service worker itself, sharing routing and templating logic whenever possible. This is much easier to do if the backend server is implemented in JavaScript. If the backend is in PHP, finding a way of translating all of the templates that it knows about into a format that could be used within a JavaScript-based service worker could be too much of a challenge.

@westonruter

This comment has been minimized.

Copy link
Collaborator

westonruter commented Jul 7, 2018

@jeffposnick Thanks a lot for the valuable information. Precaching is indeed something we haven't talked about yet, but I think that WordPress has some good tooling we can use here. For example, external scripts and stylesheets get registered with dependency system where each URL get a stable handle that does not change. Each dependency gets a version as well which is used to cache-bust the URL, but in the case of precaching we could strip the version (ver) parameter when generating the response since the service worker could handle the cache busting itself.

For service workers in WordPress what we're in general looking to do is provide the tooling that core, themes, and plugins can use to leverage service worker functionality. Some sites may want to use the app shell model, but others will not. In both cases, however, they could use the same service worker API to do what they want. If a site wants to use app shell, it may require a theme that has special support for it and which integrates with the REST API, for example. Or we may be able to build on top of AMP to create a path for arbitrary WordPress themes to be used as app shells.

In any case, I think the most critical considerations here in general for WordPress are that multiple plugins and themes may be registering their own scripts to run in the service worker and these scripts should have available an API that is easy to use. WordPress themes and plugins generally are written foremost in PHP, so there should be some PHP-level API for registering entries that eventually get exported to the runtimeCaching configuration you described. Nevertheless, if a theme/plugin wants to interface directly with the (Workbox) API in JS then they should be able to as well. WordPress developers should also be able to write raw JS for inclusion in the service worker, but in general this should be discouraged since then there would be no way to detect fetch handler conflicts. The CustomRouter you described above sounds exactly what should be included in core.

So in short, we're looking for a way to provide service worker capabilities in WordPress. We definitely want to leverage these capabilities in core wherever we can (e.g. offline authorship in Gutenberg or caching scripts/styles in the admin in the same way Google Gears used to be used in WordPress's old “Turbo mode”). The first step is to have a solid foundation to build upon. It seems like Workbox is a great fit for this.

@westonruter

This comment has been minimized.

Copy link
Collaborator

westonruter commented Jul 30, 2018

In the end, a large part of what we're looking for is a PHP API that will output Workbox JS code with calls to workbox.routing.registerRoute(), in particuar:

// Matching a Route with a String
workbox.routing.registerRoute(
    /\.(?:js|css)$/,
    workbox.strategies.staleWhileRevalidate({ /* ... */ }),
);

// Matching a Route with a Regular Expression
workbox.routing.registerRoute(
    'https://hacker-news.firebaseio.com/v0/*',
    workbox.strategies.cacheFirst({
        networkTimeoutSeconds: 3,
        cacheName: 'stories',
        plugins: [
          new workbox.expiration.Plugin({
            maxEntries: 50,
            maxAgeSeconds: 5 * 60, // 5 minutes
          }),
        ],
    }),
);

Two approaches to how there are a couple approaches to how we could achieve this in a PHP API: basic filter or fleshed-out API.

If using a basic filter, there could be something like wp_service_worker_caching_routes which manipulates an array. For example, to add the above two rules:

add_filter( 'wp_service_worker_caching_routes', function( $routes ) {
	$routes[] = array(
		'route'    => '/\.(?:js|css)$/',
		'regex'    => true,
		'strategy' => 'staleWhileRevalidate',
		'args'     => array( /* ... */ ),
	);
	$routes[] = array(
		'route'    => 'https://hacker-news.firebaseio.com/v0/*',
		'strategy' => 'cacheFirst',
		'args'     => array(
			'networkTimeoutSeconds' => 3,
			'cacheName' => 'stories',
			'plugins' => array(
				array( 'workbox.expiration.Plugin', array(
					'maxEntries' => 50,
					'maxAgeSeconds' => 5 * 60, // 5 minutes
				) ),
			)
		),
	);
	return $routes;
} );

However, this is a pretty unstructured approach and it doesn't lend itself to error checking.

So instead of just the bare filtered array, I think it would be better to have something like WP_REST_Server::register_route() to manage the registration of the cached routes in a more formalized way.

Note that we'd need for the routes registered to be JSON-serializable. The regular expression here is exported as a string but there is a regex flag. So this would not support Handling a Route with a Custom Callback since a JS function is not JSON-serializable. That is, unless we allow for JS code to be manipulated by PHP as we're in fact already doing with \WP_Service_Workers::register(). So maybe it's no so far “fetch()'ed”. But then again, more advanced usages with custom callbacks could be registered directly with \WP_Service_Workers::register().

@hellofromtonya

This comment has been minimized.

Copy link
Contributor

hellofromtonya commented Jul 30, 2018

If using a basic filter...However, this is a pretty unstructured approach and it doesn't lend itself to error checking.

I agree with both points. In addition, a filter to register caching routes mixes concerns as it allows you to add your route, but also filter out other registered routes.

So instead of just the bare filtered array, I think it would be better to have something like WP_REST_Server::register_route() to manage the registration of the cached routes in a more formalized way.

+1 here. This is a better approach as it gives developers a dedicated function with a single purpose. It makes sense as to what it does and what will happen when you invoke it.

For the plugin, it makes sense too as the registration handler can manage the registrations and abstract away any complexities. It gives us more flexibility for future enhancements.

@westonruter

This comment has been minimized.

Copy link
Collaborator

westonruter commented Jul 30, 2018

Something important that @gravityrail also has pointed out in regards to using Workbox in WP is that we should avoid tying the WordPress API inextricably from Workbox. In other words, it should be possible to switch from Workbox to another framework in the future if we need to without headaches of backwards compatibility. In Gutenberg, for example, React is not directly exposed; instead it exposes React APIs via wp.element, and this allows for the underlying implementation to be changed in the future if needed.

Given that Workbox is generally very declarative (as React is incidentally), I think this is already greatly mitigated. Given the data structure in #5 (comment) it would be straightforward to use this as the basis for a non-Workbox framework. So I feel good about this in terms of the PHP API. In terms of JavaScript, however, we need to look some more…

In #37 I opened a PR to try using Workbox in WP. It includes Workbox with a standard import from the CDN (which I note we'd likely need to eliminate for privacy concerns):

// @todo The Workbox sources will probably need to be installed locally to avoid the external request(s).
$script = sprintf(
"importScripts( %s );\n",
wp_json_encode( 'https://storage.googleapis.com/workbox-cdn/releases/3.4.1/workbox-sw.js', 64 /* JSON_UNESCAPED_SLASHES */ )
);

This introduces workbox into the service worker namespace and any JavaScript added to the service worker could then access it. If we ever move away from Workbox in core, then we'd have to maintain this workbox namespace for backwards-compatibility. So instead, as @gravityrail specifically suggested, we could create a WordPress wrapper API to interface with Workbox: instead of interacting with workbox directly we could interact with wp.serviceWorker for example.

@miina miina self-assigned this Jul 31, 2018

v0.2 automation moved this from In progress to Merged/Done Aug 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.