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

Handling service worker query vars #287

Closed
emaildano opened this issue May 26, 2020 · 29 comments · Fixed by #289
Closed

Handling service worker query vars #287

emaildano opened this issue May 26, 2020 · 29 comments · Fixed by #289

Comments

@emaildano
Copy link

Hey! Dan here from Shifter. ICYMI and for context, Shifter is a static site generator for WordPress. We're long time users of the this plugin and it's the PWA plugin we recommend to our users.

We're running into an issue with the way the server-worker is handled now and our hosting platform is unable to read the query var required to load the environment.

const QUERY_VAR = 'wp_service_worker';
/**
* Scope for front.
*
* @var int
*/
const SCOPE_FRONT = 1;
/**
* Scope for admin.
*
* @var int
*/
const SCOPE_ADMIN = 2;
/**
* Scope for both front and admin.
*
* @var int
*/
const SCOPE_ALL = 3;

To use ?wp_service_worker=1 we need to run that PHP file. Since Shifter is all about static WordPress that PHP file will never run.

To add, we are also unable to determine the MIME type when hosting in our static site environment with that URL: /?wp_service_worker=1

Is there any way to remedy this with a filter or an option? It's not common for us to create MU plugins to apply common filters to our sites but I'm not sure where to go on this one.

I can follow-up with an example URL if that is helpful. Thanks!

@westonruter
Copy link
Collaborator

The issue would be addressed if instead of /?wp_service_worker=1 the URL was instead /wp-service-worker.js?

@emaildano
Copy link
Author

The issue would be addressed if instead of /?wp_service_worker=1 the URL was instead /wp-service-worker.js?

Thanks for the super fast reply @westonruter! 🍻

I believe that would work. It would set the proper MIME type which is the solution we're looking for.

@westonruter
Copy link
Collaborator

Is the issue just the MIME type? Because there is one being sent currently:

@header( 'Content-Type: text/javascript; charset=utf-8' ); // phpcs:ignore Generic.PHP.NoSilencedErrors.Discouraged, WordPress.PHP.NoSilencedErrors.Discouraged

@emaildano
Copy link
Author

I think that entire serve request function would be skipped since it's PHP. You won't be able to send request headers and keep them after the static version is created. Would service-worker.js be an option?

If its possible through a filter or function we could work on that if updating the plugin itself isn't an option.

@westonruter
Copy link
Collaborator

It will have to be PHP one way or another. It's not going to be a static file. But it could look like a static file, with WordPress actually generating the response similar with how it handles robots.txt and favicon.ico.

The new XML sitemaps feature plugin is also doing this as wp-sitemap.xml: https://github.com/GoogleChromeLabs/wp-sitemaps/blob/149b8382ce55ff9629cadc71448abe8fd14d0870/inc/class-wp-sitemaps-index.php#L77

So there is precedence for doing this. But it depends on “pretty permalinks” being enabled, which they are by default.

Would this address the issue you're having?

cc @adamsilverstein for his thoughts on this.

@westonruter
Copy link
Collaborator

cc @swissspidy @felixarntz

@felixarntz
Copy link
Collaborator

I like the idea of using more file-like URLs for the service worker scripts using rewrites, it's a fairly common practice for sitemaps, we're also doing it in Site Kit to place "virtual" verification files.

It can have implications on some hosting configurations though, e.g. if they're configured to bypass PHP as soon as they see a "file-like" URL.

@emaildano
Copy link
Author

Hey @felixarntz 👋🏼

@westonruter That sounds like a great solution. Other plugins handle it the same way so maybe we can do that here too.

@westonruter
Copy link
Collaborator

That is concerning, however, regarding hosting environments that refuse to pass-through apparent static files for PHP to handle in the case of 404.

@emaildano can you share more details about your setup? Why does the URL matter?

@emaildano
Copy link
Author

That is concerning, however, regarding hosting environments that refuse to pass-through apparent static files for PHP to handle in the case of 404.

Do we have an example of this or is it a theory?

@westonruter

I just created this demo site. It's a fresh install with just the PWA plugin installed.

https://practical-ellis6736.on.getshifter.io/?wp_service_worker=1

@westonruter
Copy link
Collaborator

Do we have an example of this or is it a theory?

I recall this being an issue from way back in the config for VVV, where the default Nginx configuration matched URLs that had such static filenames to route to the Nginx 404 handler. This eventually got disabled: Varying-Vagrant-Vagrants/VVV#117

However, it was in place as a production-minded rule.

@emaildano
Copy link
Author

@westonruter Do you see a path for how to get the PWA plugin SW working on our environment? If there is a path I'd like to try but if not we'll have to drop support.

There are a few more things we can try on our end but they feel like hacks. Something official would be our preferred option.

Thanks!

@westonruter
Copy link
Collaborator

I suppose we can go ahead with creating the rewrite rule for /wp-service-worker.js and then we'll have to watch for whether it is causing problems on sites.

@swissspidy
Copy link
Collaborator

Does something like #289 work?

@westonruter westonruter added this to the 0.6 milestone Jun 6, 2020
@emaildano
Copy link
Author

@swissspidy I think so! 👍

@westonruter
Copy link
Collaborator

The use of wp-service-worker.js in #289 failed for me as soon as I tried:

image

I'm using the is using the https://github.com/GoogleChromeLabs/wordpressdev environment. The Nginx configuration used for the wordpress recipe has the this rule:

  location ~* \.(js|css|png|jpg|jpeg|gif|ico)$ {
    expires max;
    log_not_found off;
  }

So WordPress is bypassed entirely by Nginx for such requests, at least in this configuration. It's the same thing that VVV had (per above Varying-Vagrant-Vagrants/VVV#117).

It's also the default abridged basic setup as documented by Nginx.

So using a JS file extension will not work.

Three other options:

  1. Add a trailing slash, which will by bypass the Nginx rule, for example: /wp-service-worker.js/. But this looks ugly.
  2. Just strip off the file extension, so let the service worker be served by requests to /wp-service-worker/. There's a slight chance of a conflict with WordPress pages about "WP Service Worker".
  3. Use the REST API to serve back the service worker, similarly to how we're serving the web app manifest. For example: /wp-json/wp/v2/service-worker/. This feels like an abuse of the REST API, however, as the response is not JSON but JavaScript.

In all three options, we'll have to also serve back the Service-Worker-Allowed header along with the service worker script:

header( sprintf( "Service-Worker-Allowed: %s", home_url( '/' ) ) );

Of these three options, the second feels the least bad. Thoughts?

@swissspidy
Copy link
Collaborator

swissspidy commented Jun 11, 2020

That's just an example in the docs though. Pretty sure that WordPress hosting companies would do something like this on their environments:

# supports things like robots.txt requests
if (!-e $request_filename) {
	rewrite ^(.+)$ /index.php?q=$1 last;
}

IMO we should reach out to some hosts to see whether this is actually an issue or not, before trying to add some workarounds like the above.

WordPress does such rewrites for things like robots.txt and soon wp-sitemap.xml. I don't think we should treat wp-service-worker.js any different.

@westonruter
Copy link
Collaborator

You're right that the majority of hosts do route *.js files to WordPress. But not all. Some examples of WordPress not being routed requests to *.js files:

  • WordPress.com
  • Kinsta
  • Liquid Web
  • Hostgator

Examples where requests are routed:

  • Pantheon
  • GoDaddy
  • Dreamhost
  • WP Engine
  • Flywheel
  • Bluehost
  • SiteGround
  • Hostinger
  • Pagely
  • A2 Hosting

I determined this by finding sites by searching for ["hosted by $name" "powered by wordpress"] and then trying to navigate to /wp-service-worker.js.

Nevertheless, even WordPress.org's own support docs for Nginx config have the rule that prevents JS files from being routed to WordPress: https://wordpress.org/support/article/nginx/#general-wordpress-rules

This being the case, there's going to be a sizable number of sites that would just straight up have a service worker that would fail to install.

@westonruter
Copy link
Collaborator

westonruter commented Jul 5, 2020

What about these other three options?

  1. Add a trailing slash, which will by bypass the Nginx rule, for example: /wp-service-worker.js/. But this looks ugly.
  2. Just strip off the file extension, so let the service worker be served by requests to /wp-service-worker/. There's a slight chance of a conflict with WordPress pages about "WP Service Worker".
  3. Use the REST API to serve back the service worker, similarly to how we're serving the web app manifest. For example: /wp-json/wp/v2/service-worker/. This feels like an abuse of the REST API, however, as the response is not JSON but JavaScript.

I think I like the last option the most. Using the REST API has a great benefit: no need to register a new rewrite rule. No need to flush rewrite rules.

The only thing I don't like about it is returning non-JSON data from the REST API. The REST API does allow for this, nevertheless. Consider this POC code which serves the frontend service worker via the route /wp-json/wp/v2/service-worker/:

add_action(
	'rest_api_init',
	function () {
		register_rest_route(
			'wp/v2',
			'/service-worker',
			[
				'methods'  => 'GET',
				'callback' => function () {
					$response = new WP_REST_Response();
					$response->set_headers(
						[
							'Content-Type'           => 'text/javascript; charset=utf-8',
							'Service-Worker-Allowed' => home_url( '/' ),
							'Cache-Control'          => 'no-cache',
						]
					);
					$response->set_data( 'console.info("Hello, World!");' );
					return $response;
				},
			]
		);

		// Serve the service worker response as JS instead of JSON.
		add_filter(
			'rest_pre_serve_request',
			function ( $served, WP_REST_Response $result, WP_REST_Request $request ) {
				if ( ! $served && '/wp/v2/service-worker' === $request->get_route() ) {
					echo $result->get_data();
					$served = true;
				}
				return $served;
			},
			PHP_INT_MAX,
			3
		);
	}
);

Is that good?


Aside: It would be somewhat handy if WP_REST_Server::serve_request() would check if the sent Content-Type header, and if it is not JSON, then just skip doing:

$result = wp_json_encode( $result );

Before doing:

echo $result;

@hideokamoto
Copy link

Can we choose or customize the path to the wp-service.worker.js by ourselves?
If we can customize it by any hook, we can choose the way to load it by own WP server env.

Server A: load it by query string (currently behavior)
Server B: load it by wp API (use hook)
Server C: load it by /custom/path/service-worker.js (use hook)

Is it possible?

@swissspidy
Copy link
Collaborator

Just because we could use the REST API doesn't mean we should. Probably want to reach out to #core-restapi to ask for feedback though. Perhaps also in #hosting-community.


@hideokamoto while it would be possible to allow filtering the service worker URL, you would need to register the rewrite rules or REST API routes yourself.

@westonruter
Copy link
Collaborator

How about we just come up with a new filename extension? We can load the service worker via /wp.serviceworker.

@westonruter
Copy link
Collaborator

westonruter commented Aug 29, 2020

Per #292, WordPress will be responding with Content-Type: application/javascript, so the file extension does not matter at all (insofar the extension doesn't cause the web server to bypass PHP, as is the case for .js).

@westonruter
Copy link
Collaborator

@emaildano @hideokamoto Does #289 work for you?

@hideokamoto
Copy link

@westonruter
Thanks!
Now we're testing the branch in our hosting environment (Shifter Static).

BTW, can we detect the JavaScript file of service worker from any file like robots.txt?

@westonruter
Copy link
Collaborator

BTW, can we detect the JavaScript file of service worker from any file like robots.txt?

No, there is no standard (well-known) URL for the service worker (nor the web app manifest). For the PWA plugin, what you can do is look at the HTML source for the first argument passed to navigator.serviceWorker.register().

@hideokamoto
Copy link

Okay.
And the branch works well in our hosting env!
Could you merge it and publish it for wp.org?

Thanks.

@hideokamoto
Copy link

FYI.
We can get the sw js file from these codes.

if (window.navigator.serviceWorker.controller) {
  console.log(window.navigator.serviceWorker.controller.scriptURL)
}

@westonruter
Copy link
Collaborator

We can get the sw js file from these codes.

Ah, I thought you meant via an HTTP request like with cURL.

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

Successfully merging a pull request may close this issue.

5 participants