Site-kit-wp: Use hashes for all JS file names to avoid JS errors due to aggressive caching

[jamesozzie]

Bug Description

Using version 1.10.0 there have been separate reports of the Site Kit dashboard displaying a blank page.

Console errors below:

load-scripts.php?c=0&load[chunk_0]=jquery-core,jquery-migrate,utils,moxiejs,plupload&ver=5.4.2:8 JQMIGRATE: Migrate is installed, version 1.4.1
googlesitekit-wp-dashboard.js:41 Uncaught TypeError: Cannot read property ‘active’ of undefined
at Object.<anonymous> (googlesitekit-wp-dashboard.js:41)
at Object.<anonymous> (googlesitekit-wp-dashboard.js:41)
at __webpack_require__ (googlesitekit-wp-dashboard.js:1)
at Object.<anonymous> (googlesitekit-wp-dashboard.js:41)
at __webpack_require__ (googlesitekit-wp-dashboard.js:1)
at Module.<anonymous> (googlesitekit-wp-dashboard.js:41)
at Module.<anonymous> (googlesitekit-wp-dashboard.js:41)
at __webpack_require__ (googlesitekit-wp-dashboard.js:1)
at a (googlesitekit-wp-dashboard.js:1)
at googlesitekit-wp-dashboard.js:1

WordPress support topics:

• https://wordpress.org/support/topic/sitekit-is-showing-blank-screen-after-server-migration/ (RESOLVED)
• https://wordpress.org/support/topic/blank-white-screen-23/ (RESOLVED)

Currently troubleshooting and awaiting further information from those impacted

Additional information:

• Dashboard loads as expected for one user when using browser incognito window
• Unable to reproduce so far in support
• Site Health info provided in one support topic

- Similar support topics opened during the same version update (v.1.9.0 to 1.10.0). See #1701 and #1704

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance criteria

• All JS files Site Kit enqueues should have hashes in their file name to ensure the file name changes when its content changes and avoid "random" JS errors due to aggressive caching.
• File names should be generated in a way so that PHP logic to enqueue them can be aware of the actual names, without needing to inspect the actual files on the filesystem.
• The ver query parameter that WordPress adds by default should be omitted for those assets.

Implementation Brief

• Install the webpack-manifest-plugin package as a dev dependency.

• Modify webpack config:

  • Update output.filename to be [name].[hash].js
  • Register the ManifestPlugin plugin and use the following arguments for it:
  • fileName should be '../../../includes/Core/Assets/Manifest.php'

  • serialize should be a function that generates content for Manifest.php file. The generated content should define the Manifest class that contains only one public static property: assets. This property should be an associated array where keys are original filenames without extensions and values are filenames with hashes. For example:

    <?php
    
    namespace Google\Site_Kit\Core\Assets;
    
    class Manifest {
    
      public static $assets = array(
         "googlesitekit-data" => "googlesitekit-data.d0ec91d90a02b51a3433.js",
          ...
      );
    
    }
    

    Here is a starting point for it:

    new ManifestPlugin( {
      fileName: '../../../includes/Core/Assets/Manifest.php',
      serialize( manifest ) {
          let php = '<?php\n\nnamespace Google\\Site_Kit\\Core\\Assets;\n\nclass Manifest {\n\n\tpublic static $assets = array(\n';
    
          Object.keys( manifest ).forEach( ( key ) => {
              if ( key.match( /.js$/ ) ) {
                  php += `\t\t"${ key.replace( '.js', '' ) }" => "${ manifest[ key ] }",\n`;
              }
          } );
    
          php += '\t);\n\n}\n';
    
          return php;
      },
    } )
    

• Add /includes/Core/Assets/Manifest.php to the .gitignore file.
• Update signature of the Asset::register method and its implementations in inherited classes to accept Context $context argument.

• Update the Script::register method to check the script handle in the assets manifest list. If the script handle is available in the manifest, use its hashed representation to build a fully-qualified src for the script. Use something like this:

  $context->url( 'dist/assets/js/' . Manifest::$assets[ $this->handle ] );
  

  If the script handle is not found in the manifest, fallback to the src attribute provided in the script settings. Also make sure to check whether Manifest class exists before using it.

• Update the Assets::register_assets method to pass $this->context into $asset->register() function.
• Modify the setup_assets method of module classes:
  
  
  
  • Remove the src attribute if the script handle exists in the manifest.
  
  
  • Add 'version' => null, arg to the arguments list.
  
  

QA Brief

• This issue changes how all JavaScript files are loaded, but doesn't modify any behavior.
• It should be ensured that there are no Site Kit JavaScript files (googlesitekit-*.js) that result in a 404.
• All Site Kit JavaScript files should have names that end in some hash.

Changelog entry

• Use hashes for all JavaScript asset file names to avoid stale versions from being served on hosts with aggressive caching.

[jamesozzie]

Clearing their browsers cache resolved this issue for original poster.

For those impacted please try clearing your browsers cache or try viewing your dashboards from a Chrome browser incognito window.

[jamesozzie]

Reopening for further inspection.

[eugene-manuilov]

@felixarntz @aaemnnosttv @tofumatt

Although reported issues are resolved, I think we need to work out a better approach for our assets versioning since such issues may easily reoccur in the future if we don't change anything. The first thing that comes to my mind is to use the [hash] placeholder in the output filenames (something like [name].[hash].js) and webpack-manifest-plugin plugin to generate a manifest file with a map that contains all original filenames and hashed versions. Then we can use that manifest file to properly enqueue assets in the PHP code. Finally, when a new version of the plugin is released and users download it, they won't have such issues because browsers will have to re-load assets. Let me know what you think about it. I can write IB if it looks good to you.

PS: #1701 and #1704 are similar to this one, so I think we can consolidate these three into one ticket.

[aaemnnosttv]

@eugene-manuilov we do this for files which are loaded dynamically by webpack
https://github.com/google/site-kit-wp/blob/ced3a25d76eb39c8e5d9bf986146ab0174711e55/webpack.config.js#L129

Other files are loaded by WordPress and will receive the ver query parameter to ensure the new file is loaded when the plugin version changes. This isn't perfect though and some plugins have options to remove query parameters for "performance". This is a fairly rare case though I would say.

I'm definitely in favor of using a content-based hash over a plugin version though and errors like these shouldn't be possible anymore if all of our assets had hashes baked into the filename itself.

I have a feeling these kinds of errors are happening when users use caching/optimization plugins to combine JS, which most plugins I know of that do this strongly warn about. Even if we move to this kind of hashed file name, it wouldn't prevent plugins from doing this and causing problems but I suppose we can't guard against every possible case 😄

[eugene-manuilov]

Other files are loaded by WordPress and will receive the ver query parameter to ensure the new file is loaded when the plugin version changes.

If we switch to hashes in filenames, we can turn off ver param by passing null version to wp_enqueue_script function:

wp_enqueue_script( 'myscript', plugins_url( '/path/to/myscript.js', __FILE__ ), array( 'jquery' ), null, false );

This isn't perfect though and some plugins have options to remove query parameters for "performance".

Some aggressive proxies/CDN can cache it without query params as well.

[aaemnnosttv]

Sounds good to me @eugene-manuilov 👍
@felixarntz should we use this issue for the proposed changes or create a new one since it applies to more than one issue?

[felixarntz]

@eugene-manuilov @aaemnnosttv Let's keep working in this issue and close the other ones as duplicates of this (caused by the same problem).

[felixarntz]

@eugene-manuilov IB mostly LGTM, however I'd prefer if we kept responsibility to deal with this within the Assets namespace in Site Kit. Maybe move the Manifest output into includes/Core/Assets/Manifest.php and add the manifest URL method to the Assets class?

Potentially we could even fully handle that in the Assets classes so that other classes like the module classes wouldn't need to worry about that, i.e. for example a Script instance would automatically ensure that, if it is for a file that is in the manifest, the URL gets changed automatically. This would mean that we don't need to adjust any PHP code outside of the Assets namespace.

[aaemnnosttv]

@eugene-manuilov also, please add an estimate 😄

[eugene-manuilov]

@felixarntz yep, agree with your suggestions. IB is updated.

[felixarntz]

IB ✅

[cole10up]

Sending back to CR

During testing, I was able to validate functionality running regression on a develop build from yesterday. I rebuilt this AM and am seeing the setup page not load. I rebuilt again and it solved the issue.

We have recently published a release candidate zip (attached) that is showing the above issue.

google-site-kit.zip

Upon further digging, @tofumatt's findings below:

[felixarntz]

The problem here is the following:

• composer install generates includes/vendor/composer/autoload_classmap.php.
• npm run build[:dev] generates includes/Core/Assets/Manifest.php.
• composer install is run before npm run build[:dev] in our GitHub action to build the release, causing the class map to not include the Manifest class.
• Because of that, Site Kit tries to load JS files without hashes, although it (rightfully) includes only JS files with hashes.

Currently our dev-zip and release-zip commands don't run composer install, which means it has to be done separately. I think that's slightly problematic, since I'd expected these commands to take care of the _full_ build (right now we have to run composer install separately). Now, the problem becomes more apparent because we'll need to first run the Webpack build, then Composer, then the gulp release command.

I suggest we include composer install as a step in dev-zip and release-zip, to make sure it's run in the correct order (and in general to have that command _actually_ do the full build). I'm not sure we can just call composer install from these package.json scripts, can you look into that in the morning @eugene-manuilov @aaemnnosttv?

[cole10up]

Confirmed latest build functions properly.

No issues installing, activating and setting up SK.

Passed QA ✅