Bring back "Turbo" mode to Admin #42

miina · 2018-07-31T15:19:22Z

Fixes #34.

Using Workbox API Prototype for caching the admin assets separately and replacing the usage of load-scripts.php and load-styles.php.

Screenshots of the PR in use:

Workbox log:
Network tab:
Output in the SW script:

miina · 2018-08-02T12:14:22Z

wp-includes/class-wp-service-workers.php

+			}
+
+			// @todo This will cache both min.js and .js, however, not all the files have .min.js. Is it OK to cache all the files?
+			$routes[] = strstr( $filename, '/wp-includes' );


Should we try to precache only the files which are visibly minified (ending with .min.js) or don't have a minified version separated?

If there are two versions of a given URL, cache the min version. Otherwise, if there is no min version then cache the regular.

For example, jQuery is bundled with core and it is pre-minified so there is no minified version. The jquery-migrate script, however, does have a minified version:

https://github.com/WordPress/wordpress-develop/blob/c52e7c79b2fd6381ba89fb8c40624e6621284743/src/wp-includes/script-loader.php#L211-L212

Basically look for the presence of the $suffix variable in the registered URLs.

westonruter · 2018-08-07T02:39:39Z

wp-includes/class-wp-service-workers.php

+		);
+
+		if ( SCRIPT_DEBUG || is_preview() ) {
+			$this->register_cached_route( '/(wp-admin|wp-includes)/.*\.(?:js|css)/', self::STRATEGY_NETWORK_ONLY );


This is missing a regex flag?

westonruter · 2018-08-07T02:42:51Z

wp-includes/class-wp-service-workers.php

+			array()
+		);
+
+		if ( SCRIPT_DEBUG || is_preview() ) {


Why is_preview()? How will this ever return true given that it is the service worker response?

Right💡 Will remove.

Hmm, actually, at this moment init is called on any page if any route caching is registered, not only when serving the request of the registered service worker.

westonruter · 2018-08-07T02:46:48Z

wp-includes/class-wp-service-workers.php

+			}
+
+			// @todo This will cache both min.js and .js, however, not all the files have .min.js. Is it OK to cache all the files?
+			$routes[] = strstr( $filename, '/wp-includes' );


If there are two versions of a given URL, cache the min version. Otherwise, if there is no min version then cache the regular.

For example, jQuery is bundled with core and it is pre-minified so there is no minified version. The jquery-migrate script, however, does have a minified version:

https://github.com/WordPress/wordpress-develop/blob/c52e7c79b2fd6381ba89fb8c40624e6621284743/src/wp-includes/script-loader.php#L211-L212

Basically look for the presence of the $suffix variable in the registered URLs.

westonruter · 2018-08-07T02:50:36Z

wp-includes/class-wp-service-workers.php

+		);
+
+		if ( SCRIPT_DEBUG || is_preview() ) {
+			$this->register_cached_route( '/(wp-admin|wp-includes)/.*\.(?:js|css)/', self::STRATEGY_NETWORK_ONLY );


Is this pattern missing image extensions?

westonruter · 2018-08-07T02:54:41Z

wp-includes/class-wp-service-workers.php

 	/**
 	 * Initialize the class.
 	 */
 	public function init() {
+
+		if ( ! function_exists( 'list_files' ) ) {


The list_files() function is slow. What about instead just iterating over of the registered scripts and styles in wp_scripts() and wp_styles()? Use the ver with each registered asset as the revision.

What about the images? Do you think we should list them out one by one instead of getting dynamically? If I'm not mistaken that's how it was in the Turbo mode. Thoughts?

Changed the logic to use wp_scripts() and wp_styles(), left in the list_files() for images for now within 024eab3.

westonruter · 2018-08-07T02:55:14Z

wp-includes/class-wp-service-workers.php

+			return;
+		}
+
+		$this->register_cached_route( $routes, self::STRATEGY_PRECACHE );


It's going to be slow to as it is right now because it is going to fetch the file contents for all of the URLs to compute the hashes of each. Instead, we should use the current WordPress version as the revision, and get the list of files by looking at what is registered among default scripts and styles.

Removed using md5 hash and added using WP version by default in case the revision is not set within 84258c0. This way the API users can set the revision but it'll default to WP version if missing. Do you think the WP version is okay to use as the general default?

Yes. It's what WordPress uses as the default ver as well:

https://github.com/WordPress/wordpress-develop/blob/c52e7c79b2fd6381ba89fb8c40624e6621284743/src/wp-includes/script-loader.php#L64

…sing.

westonruter · 2018-08-07T18:02:03Z

wp-includes/class-wp-service-workers.php

+		);
+
+		if ( SCRIPT_DEBUG ) {
+			$this->register_cached_route( '/(wp-admin|wp-includes)/.*\.(?:js|css|gif|png|svg)/', self::STRATEGY_NETWORK_ONLY, array(), true );


Come to think of it, should not this condition be removed entirely? If using networkOnly then this is just passing through to the network and it would be better to just skip caching the route entirely. In other words, this if could become:

if ( ! SCRIPT_DEBUG ) { $this->precache_admin_assets(); }

westonruter · 2018-08-07T18:05:59Z

wp-includes/class-wp-service-workers.php

+		$routes = array_merge(
+			$this->get_routes_from_file_list( $admin_images, 'wp-admin' ),
+			$this->get_routes_from_file_list( $inc_images, 'wp-includes' ),
+			$this->get_admin_routes_from_dependency_list( wp_scripts()->registered ),


We should only include assets that are in the wp-admin and wp-includes directories. We should not include plugin-registered assets by default, I don't think. Or maybe we should. My concern is a plugin updated which doesn't define the ver, and so when the plugin updates and a new version of the script is included, then the service worker will not update the cache.

Sorry, I see you're doing this already in get_admin_routes_from_dependency_list!!

westonruter · 2018-08-07T18:09:40Z

wp-includes/class-wp-service-workers.php

+			if ( false === strpos( $params->src, 'wp-admin' ) && false === strpos( $params->src, 'wp-includes' ) ) {
+				continue;
+			}
+			$revision = false === $params->ver ? get_bloginfo( 'version' ) : $params->ver;


We could potentially opt-in to using the service worker for theme/plugin assets if we detect that the script is located within wp-content/themes or wp-content/plugins and then grab the version for the theme or plugin to use if the ver is empty. Or even, the ver could be combined with the version of the plugin to use as the revision.

Maybe precaching theme/plugin assets could be something that could be opted in from admin UI as a setting instead of doing this by default? Thinking that precaching theme and plugin files could cause issues (as you mentioned above) if the version is not changed when updating the code (either in WP repo or directly in the environment). Feels like it's better to leave the asset precaching by default for the admin interface only, at least for now. Thoughts?

westonruter · 2018-08-07T18:13:33Z

wp-includes/class-wp-service-workers.php

+		$routes = array();
+		foreach ( $list as $filename ) {
+			$ext = pathinfo( $filename, PATHINFO_EXTENSION );
+			if ( ! in_array( $ext, array( 'png', 'gif', 'svg' ), true ) ) {


I was going to ask about JPEG but then I saw that there are no JPEGS in core. For reference, the static assets are (via git ls-files wp-includes/ wp-admin/ | sed 's/.*\.//' | sort | uniq -c | sort -nr):

99 png 62 gif 51 css 10 scss 7 txt 6 svg 3 woff 3 ttf 3 eot 1 xml 1 crt

It doesn't make sense to include crt, eot , xml, or scss. But woff would be very useful. Also, if this is used for themes then it would be good to include the JPEG extension(s).

Thinking that maybe instead of using list_files for getting the 3 woff files we could list these out manually?

Added the three .woff files as a static list within 7132b74.

…re-turbo

miina · 2018-08-13T11:22:28Z

wp-includes/class-wp-service-workers.php

@@ -105,6 +105,15 @@ class WP_Service_Workers extends WP_Scripts {
 	 */
 	public function init() {

+		// Only init if it's for the service worker.
+		if ( ! isset( $_REQUEST['wp_service_worker'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.NoNonceVerification


Added this since if any of the API methods are used that init the WP_Service_Worker such as wp_register_service_worker() then the init logic will run on every page load instead of just when the service worker script is generated.

miina · 2018-08-13T11:23:18Z

@westonruter Let me know if anything else seems to be missing from the PR, otherwise this should be ready for review (again).

westonruter · 2018-09-11T22:09:18Z

wp-includes/class-wp-service-workers.php

+			return;
+		}
+
+		do_action( 'admin_init' );


This is an issue that I had when wanting to do_action('admin_enqueue_scripts') for the admin as it is doing wp_enqueue_scripts() for the frontend. This fails, however, when the user is not logged-in and thus the service worker would not be able to be installed at wp-login.php, for example.

westonruter · 2018-09-11T22:11:35Z

wp-includes/integrations/class-wp-service-worker-styles-integration.php

+	 * @since 0.2
+	 * @var int
+	 */
+	protected $scope = WP_Service_Workers::SCOPE_ALL;


@felixarntz Note how this had to be added to the subclass to override the variable in the base class. Something doesn't seem quite right.

@westonruter I think it makes sense to have integrations define their scope. However, the implementation I made may not be obvious enough. Currently, the default scope is SCOPE_FRONT, via the property. So in order to change it to SCOPE_ADMIN or SCOPE_ALL you need to reset the property in the child class. Two ideas:

It might make more sense to set the default scope to SCOPE_ALL.

Or alternatively, maybe it's better to not have any default scope, but instead make the WP_Service_Worker_Base_Integration::get_scope() method abstract, thus require each integration to explicitly define it. That would also make it obvious that you as the interface developer have to handle it.

This can be removed, as it is no longer necessary after #72.

westonruter · 2018-09-11T22:14:37Z

wp-includes/service-workers.php

@@ -236,4 +236,14 @@ function wp_default_service_workers( $service_workers ) {
 				);
 		}
 	}
+
+	add_action( 'wp_admin_service_worker', array( $service_workers, 'precache_admin_assets' ), 9 );


@felixarntz With the change to WP_Service_Worker_Scripts_Integration and WP_Service_Worker_Styles_Integration to add them to both the front and admin scopes, then the integration will run at wp_admin_service_worker priority 10:

https://github.com/xwp/pwa-wp/blob/57ebfa7652cd6e485056c7d11417acb726a7e87d/wp-includes/service-workers.php#L219-L221

This means that in order to set the precache flags on admin-specific scripts and styles that the flag has to be set at priority 9 of thewp_admin_service_worker action. Something is missing here.

westonruter · 2018-09-11T22:16:01Z

wp-includes/default-filters.php

@@ -18,3 +18,8 @@
 add_action( 'wp_head', 'wp_add_error_template_no_robots' );
 add_action( 'error_head', 'wp_add_error_template_no_robots' );
 add_action( 'wp_default_service_workers', 'wp_default_service_workers' );
+
+// Disable contactenation of scripts and styles on admin pages.
+foreach ( array( 'wp_default_styles', 'wp_print_scripts' ) as $filter ) {


I think this would be better done once at admin_init instead.

westonruter · 2018-09-11T22:31:59Z

wp-includes/service-workers.php

+/**
+ * Disables concatenating scripts to leverage caching the assets via Service Worker instead.
+ */
+function wp_disable_script_concatenation() {


Something we need to look into is what to do for users who don't have service workers available. In their case, we actually need to retain the concatenation functionality.

The only option that comes to mind is that when the promise returned by navigator.serviceWorker.register() resolves:

diff --git a/wp-includes/service-workers.php b/wp-includes/service-workers.php index 69a39fb..4d2029c 100644 --- a/wp-includes/service-workers.php +++ b/wp-includes/service-workers.php @@ -112,7 +112,9 @@ function wp_print_service_workers() { navigator.serviceWorker.register( <?php echo wp_json_encode( wp_get_service_worker_url( $name ) ); ?>, <?php echo wp_json_encode( compact( 'scope' ) ); ?> - ); + ).then( function() { + document.cookie = 'wordpress_sw_installed=1; path=/; expires=Fri, 31 Dec 9999 23:59:59 GMT; secure; samesite=strict'; + } ); <?php } ?> } ); }

That we can set a cookie like wordpress_sw_installed which can then be used to check for whether to proceed with concatenation.

So then in this wp_disable_script_concatenation it can first check if isset( $_COOKIE['wordpress_sw_installed'] ) before proceeding.

Note that concatenation only applies in the admin, which entails that the user is authenticated, and no full-page caching is used. Thus there won't be concerns about reading the cookie since these pages won't be cached.

@westonruter Thanks for bringing that issue out and also for the solution suggestion, good idea!

Added within ec569c4.

…talled successfully.

felixarntz · 2018-09-26T11:45:34Z

wp-includes/integrations/class-wp-service-worker-scripts-integration.php

+	 * @since 0.2
+	 * @var int
+	 */
+	protected $scope = WP_Service_Workers::SCOPE_ALL;


This can be removed, as it is no longer necessary after #72. The scripts and styles integrations are now already available in both scopes.

felixarntz · 2018-09-26T11:45:39Z

wp-includes/integrations/class-wp-service-worker-styles-integration.php

+	 * @since 0.2
+	 * @var int
+	 */
+	protected $scope = WP_Service_Workers::SCOPE_ALL;


This can be removed, as it is no longer necessary after #72.

Only add precaching route if url exists.

…c methods

…recached

…_DEBUG

miina · 2018-09-27T21:58:09Z

wp-includes/class-wp-service-workers.php

-	 * @global WP $wp
-	 *
-	 * @return int Scope. Either SCOPE_FRONT, SCOPE_ADMIN, or if neither then 0.
+	 * @todo We don't really need this. A simple call to is_admin() is all that is required.


@westonruter Not related to this specifically but a little bit related in terms of checking for self::QUERY_VAR:
Should we only init WP_Service_Worker_Scripts if the self::QUERY_VAR does exist? Otherwise wouldn't the scripts always be registered but only served if self::QUERY_VAR is set? Wouldn't the scripts need to be registered only in case of serving the service worker? Thoughts?

westonruter

In order to test this, it is important to have the plugin installed on a WordPress build install where SCRIPT_DEBUG is false.

We'll need to further refine this, especially in regards to being more selective about which assets we precache based on whether they will ever be used, or likely to be used.

Add a sample of how to cache admin assets.

c9ed3e6

miina mentioned this pull request Jul 31, 2018

Use Service Workers (as grandchild of Gears) to bring back Turbo mode in admin #34

Closed

miina added 3 commits July 31, 2018 19:03

Add todo.

49c6852

Merge Workbox API Prototype.

d484085

Leverage Workbox PHP API Prototype to precache admin assets.

19a489b

miina commented Aug 2, 2018

View reviewed changes

miina added 2 commits August 2, 2018 15:29

Precache only minified CSS files.

d3cb704

Only precache minified JS files. Add precaching for images.

e1c5b57

miina requested a review from westonruter August 3, 2018 13:32

westonruter reviewed Aug 7, 2018

View reviewed changes

miina added 5 commits August 7, 2018 16:20

Resolve conflicts.

bbf383d

Use WP version as the revision when precaching admin files.

024eab3

Use WP version as the revision when precaching if the revision is mis…

84258c0

…sing.

Fix phpcs.

2a28ba7

Remove redundant variable.

ab6f442

westonruter reviewed Aug 7, 2018

View reviewed changes

miina and others added 5 commits August 8, 2018 19:15

Update logic for SCRIT_DEBUG.

9cdb95f

Merge workbox-api and resolve conflicts.

97dbf4e

Merge Workbox API.

317d0ba

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

5eb71be

…re-turbo

Add static .woff files list for precaching.

7132b74

miina changed the title ~~[WIP] Bring back "Turbo" mode to Admin~~ Bring back "Turbo" mode to Admin Aug 13, 2018

Only run the init logic of WP_Service_Worker for service worker script.

6b267fc

miina commented Aug 13, 2018

View reviewed changes

Add unit test for precaching assets.

715f5fb

westonruter reviewed Sep 11, 2018

View reviewed changes

miina added 5 commits September 12, 2018 09:38

Avoid concatenating scripts and styles when service worker wasn't ins…

ec569c4

…talled successfully.

Merge remote-tracking branch 'origin/master' into add/restore-turbo

addc164

Merge remote-tracking branch 'origin/master' into add/restore-turbo

639e482

Merge master & resolve conflicts.

e5f1b06

Move precaching admin assets logic to an integration instead.

0f5dee2

felixarntz reviewed Sep 26, 2018

View reviewed changes

miina and others added 12 commits September 26, 2018 09:15

Remove redundant script scope assigning.

6b03aef

Only add precaching route if url exists.

Ensure admin assets get registered for precaching

c26b141

Improve passing of args to flag_admin_assets_with_precache

50971a8

Add wp_concatenate_scripts param for debugging

294b13b

Fix method declarations for precaching_routes and caching_routes magi…

90c6c69

…c methods

Add precaching for emoji script

c964d42

Serve admin service worker via admin-ajax

6f6d528

Do admin_enqueue_scripts action and include Gutenberg scripts among p…

2afe160

…recached

Only include WP_Service_Worker_Admin_Assets_Integration if not SCRIPT…

3f0003d

…_DEBUG

Short-circuit register_emoji_script when appropriate

38a2acb

Prevent precaching external asset dependencies

9b95a04

Precache tinymce assets

4bd5467

miina commented Sep 27, 2018

View reviewed changes

westonruter approved these changes Sep 27, 2018

View reviewed changes

Ensure current screen is set before doing admin_enqueue_scripts

c558d8c

westonruter merged commit 16cd551 into master Sep 27, 2018

westonruter deleted the add/restore-turbo branch September 27, 2018 22:46

westonruter mentioned this pull request Oct 17, 2018

Ensure Turbo mode works seamlessly with force_uncompressed_tinymce in WP 5.0 #91

Open

westonruter added service-workers and removed service-workers labels Feb 25, 2019

westonruter added this to the 0.2 milestone Apr 16, 2019

miina mentioned this pull request Dec 3, 2019

Facilitate offline editing in Gutenberg via caching and background sync for REST API responses #47

Open

Bring back "Turbo" mode to Admin #42

Bring back "Turbo" mode to Admin #42

Conversation

miina commented Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miina Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miina Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

miina commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

miina commented Jul 31, 2018 •

edited

Loading

miina Aug 7, 2018 •

edited

Loading

miina Aug 13, 2018 •

edited

Loading