-
Notifications
You must be signed in to change notification settings - Fork 99
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
Prototype for Service Workers API #14
Conversation
pwa-wp.php
Outdated
|
||
// These could be in ABSPATH . WPINC . '/script-loader.php' file. | ||
/** WordPress Service Workers Class */ | ||
require PWAWP_PLUGIN_DIR . '/wp-includes/class.wp-service-workers.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming convention for class files should follow class-wp-service-workers.php
instead of class.wp-service-workers.php
.
pwa-wp.php
Outdated
require PWAWP_PLUGIN_DIR . '/wp-includes/class.wp-service-workers.php'; | ||
|
||
/** WordPress Scripts Functions */ | ||
require PWAWP_PLUGIN_DIR . '/wp-includes/functions.wp-service-workers.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the REST API functions file naming convention (rest-api.php
), I think this file should be named just wp-includes/service-workers.php
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the filename. However, I was following the naming conventions of WP_Scripts
instead since the logic is similar. Isn't it confusing to reuse the methods and logic of WP_Scrips
and at the same time follow the naming conventions and hierarchy of REST API? Or are we generally trying to follow the logic of the REST API and can expect more similarities to the REST API? Or perhaps the WP_Scripts
and WP_Styles
naming conventions are to be changed at some point, too ( e.g. class-*
instead of class.*
)? Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guessing that following the REST API naming conventions is relevant since with Service Workers (as with REST API) we're sending a HTTP response not just managing the scripts as in case of WP_Script
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The REST API is newer to core, whereas the Dependencies system is older. So generally the newer stuff has better naming conventions than the older stuff. That's why I looked to the REST API for precedence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know, thanks for clarifying.
pwa-wp.php
Outdated
/** WordPress Scripts Functions */ | ||
require PWAWP_PLUGIN_DIR . '/wp-includes/functions.wp-service-workers.php'; | ||
|
||
add_action( 'wp_print_footer_scripts', 'wp_print_service_workers' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also include admin_print_footer_scripts
and customize_controls_print_footer_scripts
actions here.
pwa-wp.php
Outdated
add_action( 'wp_print_footer_scripts', 'wp_print_service_workers' ); | ||
|
||
// Alternative for this could be in wp-includes/functions.php. | ||
add_action( 'template_redirect', 'pwawp_maybe_display_sw_script' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the REST API, this can be done at the parse_request
action, which is where rest_api_loaded()
is then called.
pwa-wp.php
Outdated
*/ | ||
function pwawp_add_sw_rewrite_rules() { | ||
add_rewrite_tag( '%wp_service_worker%', '(0|1)' ); | ||
add_rewrite_tag( '%scope%', '([^&]+)' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about combining these two tags? Since the wp_service_worker
would always get the value of 1
, could we not let the scope
be the value of that query var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm if I understand correctly:
Do you mean that we could ignore wp_service_worker
as a separate query var since it's always 1
and just use scope
, for example by using the following rewrite rule instead and just adding a rewrite tag for scope
?
add_rewrite_rule( '^wp-service-worker.js\\?scope=([^&]+)?', 'index.php?scope=$matches[1]', 'top' );
Added the wp_service_worker
tag separately since in case the pretty permalinks aren't enabled then we'd need to check the wp_service_worker=1
separately, otherwise the logic will always run if $_GET['scope']
is set, even if the wp_service_worker
is not.
Thoughts?
(Likely that you meant something else by the comment.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you meant the opposite -- that we could just use wp_service_worker
, for example:
add_rewrite_tag( '%wp_service_worker%', '([^&]+)' );
where scope is the value of wp_service_worker
and use
add_rewrite_rule( '^wp-service-worker.js\\?scope=([^&]+)?', 'index.php?wp_service_worker=$matches[1]', 'top' );
?
In this case as well in case of non-pretty permalinks this wouldn't work with the current registering URL structure, unless we would use the return URL as ?wp_service_worker=$scope
for the return URL in case of non-pretty permalinks. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I meant the latter, just using the one wp_service_worker
query var. If pretty permalinks are disabled, then the URL would be /?wp_service_worker=$scope
.
pwa-wp.php
Outdated
// Alternative for this could be in wp-includes/functions.php. | ||
add_action( 'template_redirect', 'pwawp_maybe_display_sw_script' ); | ||
|
||
add_action( 'init', 'pwawp_add_sw_rewrite_rules' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create a new file called wp-includes/default-filters.php
where we put all of the filter and action hooks in the same way they are done in core. This will make it easier for a merge.
pwa-wp.php
Outdated
/** | ||
* Register rewrite rules for Service Workers. | ||
*/ | ||
function pwawp_add_sw_rewrite_rules() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per below, let's put this in wp-includes/service-workers.php
.
* | ||
* @var array | ||
*/ | ||
public $scopes = array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this $scopes
needed? There is already scope
data attached to each registered service worker, so then when getting the scripts to print when rendering the service worker it can just iterate over all the scripts and gather up the matching scope. If there are none matching, then return nothing with a 404.
By eliminating the var, then we don't have to keep track of the scopes in two different places, such as when a service worker script is unregistered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I was originally thinking of using only $scopes
instead of using handles, however, this way we wouldn't be able to leverage the existing do_items()
method which takes care of the $deps
as well.
|
||
// @codingStandardsIgnoreLine | ||
echo $this->output; | ||
exit; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the exit
can be removed from here (and above) in favor of doing it in what is currently named pwawp_maybe_display_sw_script()
(which I proposed could be named service_worker_loaded()
). This would be similar to how rest_api_loaded()
works and it would allow for this method to be unit-tested.
* | ||
* @param string $scope Scope of the Service Worker. | ||
*/ | ||
public function do_service_worker( $scope ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be better named serve_request
for parity with \WP_REST_Server::serve_request()
. See rest_api_loaded()
.
} elseif ( 0 === strpos( $url, $admin_url ) ) { | ||
$file_path = ABSPATH . 'wp-admin' . substr( $url, strlen( $admin_url ) - 1 ); | ||
} else { | ||
$file_path = ABSPATH . substr( $url, strlen( $this->remove_url_scheme( $base_url ) ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added the else
here for now.
Wondering if we should restrict the SW file path to validating against WP_CONTENT_DIR
only? Then the potential default core WP SW scripts could be registered via WP_Service_Workers::register_default_service_workers
which would skip the check for get_validated_file_path
. Not sure if plugins/themes should be able to register scripts that are outside of WP_CONTENT_DIR
. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since plugins and themes would only enqueue their scripts that are inside WP_CONTENT_DIR
I think it is safe to assume that the same would be true for service worker scripts.
wp-includes/service-workers.php
Outdated
} | ||
|
||
if ( get_option( 'permalink_structure' ) ) { | ||
return add_query_arg( compact( 'scope' ), site_url( '/wp-service-worker.js', 'relative' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need a separate case and rewrite for pretty permalinks at all? This URL wouldn't actually be directly used anyway in browser. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. If we can avoid the rewrite rules then we can save headaches. I think it is fine to just use the non-pretty URLs.
wp-includes/service-workers.php
Outdated
return add_query_arg( array( | ||
'scope' => $scope, | ||
'wp_service_worker' => 1, | ||
), site_url( '/', 'relative' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was testing and debugging it more locally with VVV and it seems to always return the correct relative path in case of https://src.wordpress-develop.test
. This is how I see the added SW:
with
@westonruter Are you still seeing the full URL instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per above, it's because I was using WP-CLI and not WP normally 😊
* @param mixed $scope Optional. Scope of the service worker. Default relative path. | ||
* @return bool Whether the item has been registered. True on success, false on failure. | ||
*/ | ||
public function add( $handle, $path, $deps = array(), $ver = false, $scope = null ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something that just came to mind: what if a script needs to be “enqueued” in multiple scopes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed the changes for registering scopes as an array.
} elseif ( 0 === strpos( $url, $admin_url ) ) { | ||
$file_path = ABSPATH . 'wp-admin' . substr( $url, strlen( $admin_url ) - 1 ); | ||
} else { | ||
$file_path = ABSPATH . substr( $url, strlen( $this->remove_url_scheme( $base_url ) ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since plugins and themes would only enqueue their scripts that are inside WP_CONTENT_DIR
I think it is safe to assume that the same would be true for service worker scripts.
$scope = site_url( '/', 'relative' ); | ||
} | ||
|
||
if ( false === parent::add( $handle, $path, $deps, false, $scope ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing $scope
as the args, I think it should pass compact( 'scope' )
. This will allow other args to be added in the future in addition to the scope.
Also, maybe a script should be able to added to multiple scopes as I mentioned above, so maybe it should be scopes
instead.
|
||
$scopes = array(); | ||
foreach ( $this->registered as $handle => $item ) { | ||
$scopes[] = $item->args; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per above, I think we should consider other args being supplied and for multiple scopes to be registered for one script. So this could instead be:
$scopes = array_merge( $scopes, $item->args['scopes'] );
|
||
// Get handles from the relevant scope only. | ||
foreach ( $this->registered as $handle => $item ) { | ||
if ( $scope === $item->args ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per above, I think args
should be preserved as an array to allow additional args in the future. Also, maybe a script should be in multiple scopes. So in that case, this could be:
if ( in_array( $item->args['scopes'], $scope, true ) ) {
@westonruter Started looking into modifying the forked repo of Super Progressive Web Apps and ran into some issues right away with the current API implementation:
I recall seeing other plugins using similar logic for registering SW in case of dynamic data. Currently the content of the registered SW static files is taken by the file path. One option could be that we'd still use the URL instead of the path here and would be able to get the dynamic content via HTTP request. Alternatively we could add a method that allows registering the SW logic via PHP method and adds the output of the method to the final output. Thoughts? |
@miina Thanks for raising this question about dynamically-generated JS. I suppose we need something like It seems we need to be able to register a service worker that either points to a file or the JS contents are provided for you instead. I believe this could be modeled in I think we should definitely avoid making HTTP requests to fetch the scripts for the service worker. Maybe there should be a wp_register_service_worker( $handle, array(
'scopes' => array( /* ... */ ),
'deps' => array( /* ... */ ),
'path' => $path_to_file,
// or
'script' => '// JS goes here',
) ) |
Alternatively, I see that the Mozilla Service Worker Manager plugin uses: $swmgr->sw()->add_content( $callback_function ); Where the So then the function signature could be: wp_register_service_worker( $handle, $callback, $deps, $scopes ); |
* @return bool Whether the item has been registered. True on success, false on failure. | ||
*/ | ||
public function register( $handle, $src, $deps = array(), $scopes = array() ) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check here for scope collisions? That is, what happens if two entities call register on the same scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is by design that two scripts can have the same scope. In fact this is the reason for this core API in the first place, to allow plugins and themes to each register their own scripts to run in the same scope as others'.
Great progress! We should discuss some of the fundamental aspects of the implementation in the context of the Service Worker API Spec : https://w3c.github.io/ServiceWorker/ |
wp-includes/service-workers.php
Outdated
foreach ( $scopes as $scope ) { | ||
?> | ||
if ( navigator.serviceWorker ) { | ||
window.addEventListener('load', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the if
statement and addEventListener
call should be placed outside the foreach
loop, as otherwise the code inside will get called multiple times unnecessarily. So like:
if ( navigator.serviceWorker ) {
window.addEventListener('load', function() {
<?php foreach ( $scopes as $scope ) : ?>
...
wp-includes/service-workers.php
Outdated
* @param array $query_vars Query vars. | ||
* @return array Query vars. | ||
*/ | ||
function wp_add_sw_query_vars( $query_vars ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better named as wp_add_service_worker_query_var()
. I don't think we need to abbreviate, since the other instances of service worker aren't abbreviated.
parent::__construct(); | ||
|
||
if ( ! class_exists( 'WP_Filesystem' ) ) { | ||
require_once ABSPATH . '/wp-admin/includes/file.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be moved into do_item
as well, and then the __construct
method can be eliminated.
return; | ||
} | ||
|
||
// @codingStandardsIgnoreLine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this, let's do:
echo $this->output; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
} else { | ||
/* translators: %s is file URL */ | ||
$this->output .= sprintf( esc_html( "\n/* Source: %s */\n" ), esc_url( $obj->src ) ); | ||
$this->output .= $wp_filesystem->get_contents( $this->get_validated_file_path( $obj->src ) ) . "\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder actually if we need to use $wp_filesystem->get_contents()
and shouldn't just use file_get_contents()
? Core actually just uses file_get_contents()
when it needs to get a file from the filesystem, for example:
I don't see any core uses of calls to $wp_filesystem->get_contents()
at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saw that as well, wasn't sure if it's because the @file_get_contents
was in use already before the WP_Filesystem
or if it's just OK for core.
If that's OK within core then we can just remove using the WP_Filesystem
and add ignore for the coding standards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's do that. The only reason for the coding standards warning is the usage of this function to make HTTP requests. Since we are validating the path is local to the filesystem, the coding standards warning does not apply.
$this->output .= sprintf( esc_html( "\n/* Source: %s */\n" ), esc_url( $obj->src ) ); | ||
|
||
// @codingStandardsIgnoreLine | ||
$this->output .= @file_get_contents( $this->get_validated_file_path( $obj->src ) ) . "\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes 4 different warnings, added // @codingStandardsIgnoreLine
.
Hi there, I'm the author of the plugin "Progressive WordPress". I just went through all your comments and your code and I'm really amazed about the progress and the decissions you made. Here are some thoughts: As proposed I think it would be awesome to either pass a callback or a file to Yes, there could be a conflict between different constants with the same name when mergin the scripts toghether. But I don't think thats a big problem. As always, we as plugin or theme devs need to prefix such things. |
@nico-martin Thanks! What do you think would make sense for a next step to validate the implementation here? What comes to mind is that you could create a branch of your plugin to try plugging in the core API proposed by this plugin. If all of the use cases of your plugin are accounted for by this PR, then I'd say it's good to merge and we can move on to other plugins to verify it accounts for their use cases as well. |
@nico-martin @westonruter Just for information that here is a PR that starts the implementation by registering the service worker of the plugin Progressive Wordpress almost as it is (with some small modifications such as wrapping the SW logic into IIFE): xwp/progressive-wordpress#1. |
@miina yes, I've seen your implementations and I will try them. |
Additionally to my comment on #8 I did run some tests with the API: https://github.com/SayHelloGmbH/progressive-wordpress/blob/pwawp/Classes/class-serviceworker.php#L39 |
Also let $scope argument be optional.
nocache_headers(); | ||
|
||
if ( ! in_array( $scope, $this->get_scopes(), true ) ) { | ||
status_header( 404 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #26. This should perhaps not return a 404
but rather return a 200
and a service worker script that simply unregisters itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(My suggestion won't work.)
Demonstration of the service worker API from this PR being used in the AMP plugin to cache the CDN assets: ampproject/amp-wp#1261 |
In a subsequent PR let's re-consider the scope, and whether we should allow arbitrary scopes or restrict to just |
Fixes #7.
Currently mainly just a skeleton for registering service workers, however, it is functional.For example you could add
test.js
to your plugin root directory and then callwp_register_service_worker( 'test', '/wp-content/path-to-your-file/test.js' );
-- it should work.Updated description:
Allows registering static SW scripts via
wp_register_service_worker( $handle, $src, $deps = array(), $scopes = array() )
where:Concatenates the scripts dynamically based on scopes, the following URL is used for registering:
/?wp_service_workers=$scope
and registers service worker per each scope. Default scope is the relative site URL;In case of using a source file and not a callback method: The content of SW scripts is received from the absolute path by using
$wp_filesystem->get_contents()
and not via HTTP request meaning that the content has to be static.