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

Replace arbitrary service worker script scopes with just three: all, front, and admin #27

Merged
merged 5 commits into from Jul 11, 2018

Conversation

Projects
None yet
9 participants
@westonruter
Contributor

westonruter commented Jul 10, 2018

In #14 the ability was added for a service worker script to be registered with arbitrary scopes. This ability caused a problem (#26) whereby if a plugin registered a service worker script to an arbitrary scope and then that plugin was deactivated, that the service worker would be left installed indefinitely.

In reality, I believe only two distinct service workers would be relevant in a WordPress context: the frontend (home_url('/')) and the wp-admin (admin_url('/')). These are really the two separate environments in WordPress. On the frontend, all requests get routed by WP_Rewrite and loaded via /index.php and it makes sense that all service worker scripts used on the frontend should similarly get the corresponding / scope. A frontend service worker would likely be used to cache frontend assets (such as for an app shell), which should be very different from how caching is handled in the admin. In the admin it would make sense for there to be scripts registered which are specific to the apps running in that context (e.g. Gutenberg) which would be needless to be included in the service worker on the frontend. Also, it is important to note that the frontend of a site and the WP admin can be served from different domains (e.g. example.com and example.wordpress.com/wp-admin) and so again, there is a natural differentiation between them.

Because of this potential for differing front/admin domains, I think it is important for consistency to always have a service worker registered at / and another at /wp-admin/. Otherwise, it could be that a plugin registers a script to run at the / scope and would then expect it to run at /wp-admin/ but when the the admin is accessed on a different, this wouldn't happen as expected: the service worker installed on the home origin would not be the same service worker installed when accessing the admin. A separate admin service worker would have to get installed anyway.

So I think we should always register a service worker at the home_url( '/' ) scope for the frontend, and another service worker scope at site_url( '/wp-admin/' ). When the home and site URLs are the same, then the admin service worker could be installed when accessing the frontend, but we can limit this installation to only happen when the user is logged in; this installation can also happen when accessing the login screen so the admin service worker installs while logging in.

Finally, it should also be possible register a service worker script to run in both the front and admin contexts. For example in Automattic/amp-wp#1261 I added a service worker to the AMP plugin to cache AMP CDN assets (e.g. so that they can be loaded while developing offline). Such a service worker logic is relevant to the frontend and wp-admin, and such a script a script can be registered with an all scope, instead of a front or admin scope.

Usage

// Register script to only run on the frontend, with dependency on some other app-shell SW script.
wp_register_service_worker(
    'foo', // Handle.
    plugin_dir_url( __FILE__ ) . 'foo.js', // Source.
    array( 'app-shell' ), // Dependency.
    WP_Service_Workers::SCOPE_FRONT // Scope.
);

// Register script (here via render callback instead of URL) to only run only in the admin.
wp_register_service_worker( 
    'bar', 
    function() { return 'console.info("Hello admin!")'; }, 
    array(), // No deps.
    WP_Service_Workers::SCOPE_ADMIN
);

// Register script with to run in both the frontend and wp-admin.
wp_register_service_worker( 'baz', plugin_dir_url( __FILE__ ) . 'baz.js', array(), WP_Service_Workers::SCOPE_ALL );

// The default values for $deps and $scope are array() and WP_Service_Workers::SCOPE_ALL  respectively, so this is same as previous.
wp_register_service_worker( 'baz', plugin_dir_url( __FILE__ ) . 'baz.js' );

Changes

  • Replace arbitrary scopes with just three: all, admin, and front.
  • Install service workers on login screen and other non-front/admin screens
  • Serve scripts registered to 'all' scope in service worker requests for both 'front' and 'admin'.
  • When requesting service worker other than front/admin respond with 400 error.
  • Remove no-cache header which conflicted with logic to handle not-modified responses.
  • Improve comments in service worker before each script is printed, indicating handle and error.
  • Let remove_url_scheme and get_validated_file_path methods be protected.
  • Limit installation of service workers for front and admin based on whether they can be installed (on basis of current origin and whether user would access admin).

Closes #26. See #7.

westonruter added some commits Jul 10, 2018

Replace arbitrary SW script scopes with just three: all, admin, and f…
…ront

* Serve scripts registered to 'all' scope in service worker requests for both 'front' and 'admin'.
* When requesting service worker other than front/admin respond with 400 error.
* Remove no-cache header which conflicted with logic to handle not-modified responses.
* Improve comments in service worker before each script is printed, indicating handle and error.
* Let remove_url_scheme and get_validated_file_path methods be protected.
* Limit installation of service workers for front and admin based on whether they can be installed (on basis of current origin and whether user would access admin).
@westonruter

This comment has been minimized.

Contributor

westonruter commented Jul 10, 2018

I terms of implementation, maybe instead of using a custom scope script arg we should re-use the dependency group instead, where 0 is all, 1 is frontend, and 2 is wp-admin. It wouldn't make any difference in practice. It's just we're not using the $group argument inWP_Service_Workers::do_items() and instead are manually collecting the handles in $scope_items.

@westonruter westonruter changed the title from Replace arbitrary SW script scopes with just three: all, front, and admin to Replace arbitrary service worker script scopes with just three: all, front, and admin Jul 10, 2018

@JJJ

This comment has been minimized.

JJJ commented Jul 10, 2018

Login/Register/Activate pages are unique in that they are neither front nor admin.

Could we foresee API specific service workers? If so, is REST unique from XML-RPC? Should WPGraphQL do the same front/admin specific scope instead of providing their own?

RSS/feeds? (I know support is waning.)

Any concerns about admin-ajax, trackbacks, cron, comments-post, or other core files that are anomalies in how they are loaded and what they load with them?

@gravityrail

This comment has been minimized.

Collaborator

gravityrail commented Jul 10, 2018

RSS/feeds? (I know support is waning.)

Feeds don't support JavaScript :)

@westonruter

This comment has been minimized.

Contributor

westonruter commented Jul 10, 2018

@JJJ:

Login/Register/Activate pages are unique in that they are neither front nor admin.

The idea here is that if you are accessing the login screen or the register/activate screen that it is likely you will soon be accessing the admin. This being the case, the admin service worker should get installed while you're on login/register/activate so that it's all primed by the time you do access the admin.

Could we foresee API specific service workers? If so, is REST unique from XML-RPC? Should WPGraphQL do the same front/admin specific scope instead of providing their own? ¶ Any concerns about admin-ajax, trackbacks, cron, comments-post, or other core files that are anomalies in how they are loaded and what they load with them?

I'm not sure. I'm thinking specifically in the context of the service worker that is running when you're at / vs /wp-admin/ and the fetch events it might handle. The REST API runs under home_url( '/wp-json/' ) so requests to it would run through the front scope as proposed above. This makes sense to me for example in the context of an app shell PWA that needs to have control over the URL space over the frontend of the site and across the the REST API endpoints in order to effectively serve a site offline. RSS feeds would be included here, but I doubt that modern JS applications would be be fetching posts via RSS instead of via the REST API. The fetch handlers will need to whitelist which routes they handle, so they should be ignoring xmp-rpc.php and other such special endpoints.

In #5 there is an investigation into using Workbox to provide an abstraction layer to the service worker API which we can then in turn provide an PHP API abstraction in WordPress. We'd probably want to fail with as _doing_it_wrong() for any fetch handlers that want to proxy the network requests such special WordPress endpoints.

@nico-martin

This comment has been minimized.

Contributor

nico-martin commented Jul 10, 2018

Interresting feed so far. One thing first. I don't think we need to find solutions for all usecases right now. We only need to think about the scopes of serviceworkers.
On a regular installation we would have the frontend scope https://site.com/ and the admin scope https://site.com/wp-admin/. So the frontend scope will cover the admin scope as well.
But to split it makes sense because sometimes we have different urls and not all visitors need all the admin SW stuff (but sll admins do need the frontend SW stuff).

@JJJ We don't need API specific ServiceWorkes. As @gravityrail suggested you can't register a SW on an API request. But I think you are not talking about API specific ServiceWorkes but more about how we handle API requests inside the frontend ServiceWorker, right?

I'd say we shouldn't touch them. APIs should always provide the latest informations. Let's say I'm writing an SPA toghether with the WP Rest API. I'd rather have a 404 response than an offline/cached version of the Endpoint. If I have the 404 I can decide what my SPA should do. Otherwise I have no way to check wether the request was successfull and my data is up to date or I recieved outdated data.

Maybe there are some rare usecases where an offline-API could make sence. But then it would make more sense to create a plugin that adds those events to the frontend-SW.

@nico-martin

This comment has been minimized.

Contributor

nico-martin commented Jul 10, 2018

I'm a bit concerned about the all scope.
Let's say you have some SW-JS that handles the fetch events for all JS files.

workbox.routing.registerRoute(
    new RegExp('.*\.js'),
    workbox.strategies.networkFirst()
);

Lets say we put that inside the all scope and we are currently inside the wp-admin area. That would mean we have two SW registered in our browser and both have the same event for handling JS files. Doesn't make sense, right?

Speaking of that. What happens if we have an all scope SW-JS and a admin scope SW-JS?

// all scope
workbox.routing.registerRoute(
    new RegExp('.*\.js'),
    workbox.strategies.cacheFirst()
);
// admin scope
workbox.routing.registerRoute(
    new RegExp('.*\.js'),
    workbox.strategies.networkFirst()
);

In that case w e would have 3 JS events in two ServiceWorkers. We have the cacheFirst strategy from the FrontentSW, we have the cacheFirst strategy from the AdminSW and we have the networkFirst strategy from the AdminSW. Thats all a bit confusing..

@felixarntz

This comment has been minimized.

Collaborator

felixarntz commented Jul 10, 2018

I am in favor of using the group approach with integers, however I'd strongly advise for using constants there, either globally defined (such as SW_GROUP_ALL, SW_GROUP_FRONTEND, and SW_GROUP_ADMIN) or, better even, in a "enum-like" class (such as Service_Worker_Group::ALL, Service_Worker_Group::FRONTEND, and Service_Worker_Group::ADMIN). Having constants makes it obvious which values are available, allows for autocompletion support and prevents typos from causing unexpected behavior because it would trigger a notice instead.
Also, in terms of common practices, it would make sense to use values that work correctly with bit operators, for example, the value for Service_Worker_Group::ALL should be the value of Service_Worker_Group::FRONTEND | Service_Worker_Group::ADMIN. 0 should only be used for something like Service_Worker_Group::NONE (which I don't think is needed, so we should skip that here).

Regarding the scope in which those apply, I wonder whether we could be inspired by how this is handled for cookies. Either way, I think having constants available for this as well would make a lot of sense, however in this case not for consuming developers to use (like plugin developers), but more so for site owners, where they can define those relative URLs in wp-config.php, and if not, they are defined later with defaults, just like currently COOKIE_PATH etc. That may be useful especially for multisite where service workers should possibly be able to go beyond the scope of a single site.

@nico-martin

This comment has been minimized.

Contributor

nico-martin commented Jul 10, 2018

Ok. Interresting thread here:
https://stackoverflow.com/a/45285119/2949413

The behavior is well-defined: when a client page makes a request, it will trigger the fetch handlers of the controlling service worker one by one, in the order in which they were registered, until the first call is made to event.respondWith(). One one fetch event handler calls respondWith(), no other fetch event handlers will be triggered, and it's the sole responsibility of that handler to (eventually) return a Response to the client page.

I assume that applies also to multiple fetch handlers from multiple ServiceWorkers.
But does anybody know whats the order of already registered ServiceWorkers? If someone visits the frontend, the frontendSW will be registered. Then he visits the admin area, the adminSW will be registered. How can we make sure the events from the adminSW are executed before the events from the frontendSW?

@westonruter

This comment has been minimized.

Contributor

westonruter commented Jul 10, 2018

@felixarntz:

I am in favor of using the group approach with integers, however I'd strongly advise for using constants there, either globally defined (such as SW_GROUP_ALL, SW_GROUP_FRONTEND, and SW_GROUP_ADMIN) or, better even, in a "enum-like" class (such as Service_Worker_Group::ALL, Service_Worker_Group::FRONTEND, and Service_Worker_Group::ADMIN). Having constants makes it obvious which values are available, allows for autocompletion support and prevents typos from causing unexpected behavior because it would trigger a notice instead.

Yes, good idea to use constants here.

Also, in terms of common practices, it would make sense to use values that work correctly with bit operators, for example, the value for Service_Worker_Group::ALL should be the value of Service_Worker_Group::FRONTEND | Service_Worker_Group::ADMIN. 0 should only be used for something like Service_Worker_Group::NONE (which I don't think is needed, so we should skip that here).

I thought about this. And yes, we could do this, but it just seems a little bit overkill. I think FRONT could be the value 1 (b1) and ADMIN could have value of 2 (b10) and ALL could be 3 (b11). You're right that a NONEgroup wouldn't make sense.

Regarding the scope in which those apply, I wonder whether we could be inspired by how this is handled for cookies. Either way, I think having constants available for this as well would make a lot of sense, however in this case not for consuming developers to use (like plugin developers), but more so for site owners, where they can define those relative URLs in wp-config.php, and if not, they are defined later with defaults, just like currently COOKIE_PATH etc. That may be useful especially for multisite where service workers should possibly be able to go beyond the scope of a single site.

Multisite is currently handled by making sure that the scopes' paths relative to the location of the subdirectory install. So it does not use the scope of / for FRONT but rather home_url( '/', 'relative' ). Likewise, for ADMIN it uses the scope of admin_url( '/' ) to ensure it runs under /wp-admin/ (and also may be on a different domain). So is there a need for having additional configuration via constants?

Now, in regards to having a separate service worker for login, signup, activate pages (as …

In the case of the login screen I think the FRONT service worker could be running but it just probably not have anything to do, since the page wouldn't be making requests that the service worker should be intercepting. I didn't think login pages warrant having a separate scope. What would the login screen need a dedicated service worker for? It's not a page with assets that would likely need to be proxied by a service worker. I can't imagine it anyway.

@westonruter

This comment has been minimized.

Contributor

westonruter commented Jul 10, 2018

@nico-martin:

I'm a bit concerned about the all scope. ¶ Lets say we put that inside the all scope and we are currently inside the wp-admin area. That would mean we have two SW registered in our browser and both have the same event for handling JS files. Doesn't make sense, right? Speaking of that. What happens if we have an all scope SW-JS and a admin scope SW-JS? ¶ In that case w e would have 3 JS events in two ServiceWorkers. We have the cacheFirst strategy from the FrontentSW, we have the cacheFirst strategy from the AdminSW and we have the networkFirst strategy from the AdminSW. Thats all a bit confusing..

Note that all just means front and admin. Also, only one service worker can be running at a time, and it is the service worker with the longest-matching scope that will be running on a given URL. So if the front service worker is installed at the / scope and the admin service worker is installed with the /wp-admin/ scope, then only the admin service worker will be running when the user accesses the WP admin. Thus there will not be the concern about there being multiple fetch handlers listening, because only one service worker will be running.

@JJJ

This comment has been minimized.

JJJ commented Jul 10, 2018

I can imagine there being a need for multiple front scopes. It’s not uncommon for plugins to define some “root” page/slug for themselves to naturally reduce their scope already. bbPress does this with forums. BuddyPress does this with each of its components. EDD has specific pages for products/cart/success/fail.

BuddyPress and bbPress also rely on the vanilla login forms, but both force a redirect back to the front for Subscriber role users who shouldn’t have access to wp-admin. That makes me question the assumption that those pages are admin-scope.

@westonruter

This comment has been minimized.

Contributor

westonruter commented Jul 10, 2018

@JJJ If a plugin were to add a service worker script scoped to their own path on the site, then I'd worry that it would cause unexpected results. In particular, let's say a theme wants to add a service worker script to handle caching of stylesheets and this service worker is registered at the / scope:

self.addEventListener( 'fetch', ( event ) => {
    const url = new URL( event.request.url );
    if ( /\/wp-content\/themes\/example\/.*\.css$/.test( url.pathname ) ) {
        // Handle response.
    }
} );

But then you have another plugin (e.g. EDD) that adds offline support for the shopping cart page (for example), and it only is registered to the /store/ scope:

self.addEventListener( 'fetch', ( event ) => {
    const url = new URL( event.request.url );
    if ( /\/store\/cart\/$/.test( url.pathname ) ) {
        // Handle response.
    }
} );

The result is that the service worker registered with the /store/ scope would take control over the page and exclude the service worker registered at the / scope. Only one service worker can handle fetch events on a given page, and the service worker that does runs is the one that has the longest-matching scope for the current window.location. So in this case the theme's logic (above) for handling the caching for stylesheets would not run because the store-specific service worker is more specific.

See more at https://stackoverflow.com/a/41707357/93579:

It's possible to register multiple service workers with different scopes for a given origin. (Aside: If you try to register multiple service workers with the same scope for a given origin, even if the service workers have different URLs, the subsequent registrations will be treated like service worker updates, not as separate registrations.)

If multiple service workers are registered, then in order to determine which one controls a give client page, the scopes are checked. The service worker whose registered scope is most specific, i.e. is the longest matching scope, is the one which will control a given client page.

All of this to say, however, a theme wanting to add fetch handler at the root can work well with a plugin wanting to add a fetch handler for an e-commerce plugin wanting to handle requests under /store/. The fetch handlers just need to make sure they are explicit about the URLs they are handling. If this is done, then they can all be concatenated into a single front service worker which is registered at the / scope:

// From theme:
self.addEventListener( 'fetch', ( event ) => {
    const url = new URL( event.request.url );
    if ( /\/wp-content\/themes\/example\/.*\.css$/.test( url.pathname ) ) {
        // Handle response.
    }
} );

// From store plugin:
self.addEventListener( 'fetch', ( event ) => {
    const url = new URL( event.request.url );
    if ( /\/store\/cart\/$/.test( url.pathname ) ) {
        // Handle response.
    }
} );

(The event.request.url is not the current window.location but rather the URL for the given HTTP request being made, even to URLs that are outside the current scope, including to entirely different domains. The SW scope controls which service worker gets installed for a given HTML document you access.)

Note that the fist fetch handler that handles an event will cause all subsequent handlers to be skipped. So it's important that the URL pattern matches be as specific as possible. In #5 there's an exploration into using Workbox to facilitate registering the routes and their caching strategies in a way that we can detect fetch collisions which would otherwise be silently skipped.

I think this explains why having a single front scope is advantageous and that allowing for arbitrary scopes on the frontend would just increase the likelihood for unexpected behavior.

@mehigh

mehigh approved these changes Jul 10, 2018

Looks great! I'm really happy with the direction this is going into.

@westonruter

This comment has been minimized.

Contributor

westonruter commented Jul 10, 2018

I'll merge this in ~10 hours unless there are any objections. Of course we will continue to iterate in other PRs. Hoping that this issue of the scope can be largely settled here, however.

@JJJ

This comment has been minimized.

JJJ commented Jul 10, 2018

@westonruter that explanation helps immensely. Thanks!

@nico-martin

This comment has been minimized.

Contributor

nico-martin commented Jul 10, 2018

Sounds good to me. Especially the service worker hierarchy. I didn't know that, but it makes sense.

@westonruter

This comment has been minimized.

Contributor

westonruter commented Jul 10, 2018

I am in favor of using the group approach with integers, however I'd strongly advise for using constants there, either globally defined (such as SW_GROUP_ALL, SW_GROUP_FRONTEND, and SW_GROUP_ADMIN) or, better even, in a "enum-like" class (such as Service_Worker_Group::ALL, Service_Worker_Group::FRONTEND, and Service_Worker_Group::ADMIN). Having constants makes it obvious which values are available, allows for autocompletion support and prevents typos from causing unexpected behavior because it would trigger a notice instead.
Also, in terms of common practices, it would make sense to use values that work correctly with bit operators, for example, the value for Service_Worker_Group::ALL should be the value of Service_Worker_Group::FRONTEND | Service_Worker_Group::ADMIN. 0 should only be used for something like Service_Worker_Group::NONE (which I don't think is needed, so we should skip that here).

@felixarntz What do think of 1cb73ac? Switches to:

  • \WP_Service_Workers::SCOPE_FRONT (b01) instead of 'front'
  • \WP_Service_Workers::SCOPE_ADMIN (b10) instead of 'admin'
  • \WP_Service_Workers::SCOPE_ALL (b11) instead of 'all'
@postphotos

This comment has been minimized.

Collaborator

postphotos commented Jul 10, 2018

This is fantastic. Thanks, @westonruter for your efforts here! 🎉

@felixarntz

This comment has been minimized.

Collaborator

felixarntz commented Jul 11, 2018

@westonruter

Multisite is currently handled by making sure that the scopes' paths relative to the location of the subdirectory install. So it does not use the scope of / for FRONT but rather home_url( '/', 'relative' ). Likewise, for ADMIN it uses the scope of admin_url( '/' ) to ensure it runs under /wp-admin/ (and also may be on a different domain). So is there a need for having additional configuration via constants?

I think using constants would make sense, for example for a case where a subdirectory configuration would like the service workers to not act in the relative home URL scope (/my-subsite/), but rather in the network URL scope (/). On a subdirectory network, that would allow service workers to work on the full network level. I see that it might be a bit far fetched at this point, but I don't see any harm in introducing constants for this. Multisite (and multinetwork especially) is used in tons of ways that we should open up flexibility for here IMO.

What do think of 1cb73ac?

I like those, looks good to me! I just saw you went with the integer approach for scopes as well right? I wonder whether we can get rid of the front and admin strings that are still in wp_get_service_worker_url() and wp_print_service_workers() somehow.

@westonruter

This comment has been minimized.

Contributor

westonruter commented Jul 11, 2018

I like those, looks good to me! I just saw you went with the integer approach for scopes as well right? I wonder whether we can get rid of the front and admin strings that are still in wp_get_service_worker_url() and wp_print_service_workers() somehow.

@felixarntz Good catch! Updated in 36704a7.

@westonruter westonruter merged commit 6ba82ed into master Jul 11, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the update/scope branch Jul 11, 2018

@aristath

This comment has been minimized.

aristath commented Jul 11, 2018

A frontent/wp-admin/all separation sounds like a plan to me 👍

@justlevine

This comment has been minimized.

justlevine commented Jul 11, 2018

Just my (wholly unsolicited) 2 cents regarding Login/Register/Activate:
A decent amount of sites use these for the frontend as well (e-commerce, forums, social, etc), and the majority of logged-in users for those types of sites arent ever allowed to access wp-admin.

Assumingly, most in this use case who use these (instead of a plugin-provided solution) are capable enough to use a filter/action to say with group of service workers load, but don't take it for granted that sites are always going to want/need wp-admin service workers loading up.

@westonruter

This comment has been minimized.

Contributor

westonruter commented Jul 11, 2018

Yeah, in that case the admin service worker could get installed for users but it would never be used. If a filter is used to turn that off, however, then admin users wouldn't get the benefit of the service worker installing during login. Maybe that's something they'd just have to live with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment