Improve webpack cache-busting version parameter by using file contents hash #44838

chihsuan · 2024-02-21T10:39:19Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

This PR improves the cache-busting version parameter by using ?ver=<file hash> instead of a version number, which makes it so that the client does not have to re-fetch resources that haven't changed recently.

There are two kinds of resources, one is scripts that are enqueued in PHP, and the other is chunks that are generated by webpack and loaded in the frontend.

For the former, the version parameter is added to the URL by the wp_enqueue_script function, we uses the version from the asset manifest file to get the file hash. It's possible to get file hash dynamically but it may affect performance, so I chose to use the asset manifest file. The approach is also used to load wp-includes scripts in WP core.

For the latter, we simply change the output config in webpack.config.js to load the file with the hash.

The asset manifest file is generated by the dependency-extraction-webpack-plugin and contains the unique version hash calculated based on the file content. However, it only supports JS files. To support CSS files, I added a new plugin called style-asset-plugin to extract the CSS file hash and write it to the asset manifest file. It's just a simple modification of the dependency-extraction-webpack-plugin.

Please note that because the asset file is not regenerated in Webpack watch mode, you need to run pnpm build to get the new hash and when SCRIPT_DEBUG is enabled, the version parameter is replaced with file modified time.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

Create a fresh site
Set define( 'SCRIPT_DEBUG', false ); in your wp-config.php file.
Open dev tool > network tab
Go to WooCommerce
Skip core profiler and go to WooCommerce > Home
Verify that the ?ver=<hash> is set on all WooCommerce Admin JS and CSS requests, please ignore assets loaded from /assets/ path since we're not able to use webpack to get file hash.
Modify the plugins/woocommerce-admin/client/app.js (add a console.log for example)
Run pnpm run --filter @woocommerce/plugin-woocommerce build
Reload the page and confirm the <hash> value is changed
Go to /wp-admin/admin.php?page=wc-settings&tab=checkout
Confirm payment-method-promotions.js script is loaded properly like payment-method-promotions.js?ver=5a20ddfd6fe486acf2ba.
Set define( 'SCRIPT_DEBUG', true ); in your wp-config.php file.
Reload the page and confirm?ver=<timestamp> is set on all WooCommerce Admin JS and CSS requests (Ignore legacy assets from /assets/ path and /chunks)

When SCRIPT_DEBUG is enabled:

Changelog entry

Automatically create a changelog entry from the details below.

Significance

Patch
Minor
Major

Type

Fix - Fixes an existing bug
Add - Adds functionality
Update - Update existing functionality
Dev - Development related task
Tweak - A minor adjustment to the codebase
Performance - Address performance issues
Enhancement - Improvement to existing functionality

Message

Improve webpack cache-busting version parameter by using file contents hash

Comment

chihsuan · 2024-02-21T10:45:46Z

plugins/woocommerce/src/Internal/Admin/WCPayPromotion/Init.php

-			WCAdminAssets::get_file_version( 'css' )
-		);
-
-		WCAdminAssets::register_script( 'wp-admin-scripts', 'payment-method-promotions', true );


This shouldn't be called in the constructor as it will be called multiple times. Additionally, observing it in watch dev mode sometimes results in errors.

[21-Feb-2024 07:09:48 UTC] PHP Fatal error: Uncaught Exception: Could not find asset registry for wp-admin-scripts in /Users/chihsuan/Projects/woocommerce/plugins/woocommerce/src/Internal/Admin/WCAdminAssets.php:136 Stack trace: #0 /Users/chihsuan/Projects/woocommerce/plugins/woocommerce/src/Internal/Admin/WCAdminAssets.php(471): Automattic\WooCommerce\Internal\Admin\WCAdminAssets::get_script_asset_filename('wp-admin-script...', 'payment-method-...') #1 /Users/chihsuan/Projects/woocommerce/plugins/woocommerce/src/Internal/Admin/WCPayPromotion/Init.php(44): Automattic\WooCommerce\Internal\Admin\WCAdminAssets::register_script('wp-admin-script...', 'payment-method-...', true) #2 /Users/chihsuan/Projects/woocommerce/plugins/woocommerce/src/Admin/Features/Features.php(138): Automattic\WooCommerce\Internal\Admin\WCPayPromotion\Init->__construct()

Good catch!

github-actions · 2024-02-21T10:48:31Z

Test Results Summary

Commit SHA: 595e99f

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 38s
E2E Tests	338	0	0	9	0	347	7m 55s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

github-actions · 2024-02-21T11:44:22Z

Hi @psealock, @ilyasfoo, @woocommerce/mothra

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

psealock

nice work @chihsuan!

Just a couple comments and questions. And I am curious if this would be a better upstream contribution because it means less for Woo to maintain. Can Style Asset Plugin remain if the addition of CSS gets merged upstream directly to Dep Extraction Webpack Plugin? If so, we can merge this with a comment to safely remove once upstream changes materialise.

psealock · 2024-02-21T23:35:55Z

packages/js/internal-style-build/style-asset-plugin.js

+ *
+ * This is modified from WP dependency-extraction-webpack-plugin plugin:
+ * https://github.com/WordPress/gutenberg/tree/a04a8e94e8b93ba60441c6534e21f4c3c26ff1bc/packages/dependency-extraction-webpack-plugin
+ */


To support CSS files, I added a new plugin called style-asset-plugin to extract the CSS file hash and write it to the asset manifest file. It's just a simple modification of the dependency-extraction-webpack-plugin.

Seems like a great upstream contribution. If I'm reading correctly, you've inserted logic to find CSS files in the loop over entrypointChunks and assigned a content hash, similar to the one used for JS.

Correct! And I also removed some unnecessary extraction logic. 🙂

psealock · 2024-02-21T23:48:58Z

plugins/woocommerce/src/Internal/Admin/WCAdminAssets.php

+	 */
+	public static function register_style( $style_path_name, $style_name, $dependencies = array() ) {
+		$style_assets_filename = self::get_script_asset_filename( $style_path_name, $style_name );
+		$style_assets          = require WC_ADMIN_ABSPATH . WC_ADMIN_DIST_JS_FOLDER . $style_path_name . '/' . $style_assets_filename;


This is working, but I'm wondering why this wouldn't use WC_ADMIN_DIST_CSS_FOLDER instead of the JS one?

They are now one and the same! This would be a good opportunity to clean that up.

woocommerce/plugins/woocommerce/src/Internal/Admin/FeaturePlugin.php

Lines 107 to 108 in be4a365

$this->define( 'WC_ADMIN_DIST_JS_FOLDER', 'assets/client/admin/' );

$this->define( 'WC_ADMIN_DIST_CSS_FOLDER', 'assets/client/admin/' );

Thanks for brining this up! Do you have any suggestions? I think even though they are the same, maintaining two variables can provide flexibility.

I will change register_style to use WC_ADMIN_DIST_CSS_FOLDER.

Changed to use WC_ADMIN_DIST_CSS_FOLDER in 1c0d6c6

psealock · 2024-02-21T23:57:52Z

plugins/woocommerce/src/Internal/Admin/ShippingLabelBanner.php

-			WCAdminAssets::get_file_version( 'css' )
-		);
-
+		WCAdminAssets::register_style( 'print-shipping-label-banner', 'style', array( 'wp-components' ) );


This will be renaming the script to wc-admin-print-shipping-label-banner, I'm pretty sure nothing depends on this script, but it would be good to go through and double check we aren't renaming anything in this PR that isn't being depended on elsewhere in Core or otherwise out in the wild.

Thanks for the heads up!

I've double-checked, and only this script is renamed in this PR and also confirmed that those scripts are loaded properly, so we should be good to go.

chihsuan · 2024-02-22T05:53:49Z

And I am curious if this would be a better upstream contribution because it means less for Woo to maintain. Can Style Asset Plugin remain if the addition of CSS gets merged upstream directly to Dep Extraction Webpack Plugin? If so, we can merge this with a comment to safely remove once upstream changes materialise.

@psealock Yes, good idea. 👍 It should be possible to contribute upstream. I'll create a follow-up issue and add a comment to the Style Asset plugin.

Edit: Issue created: https://github.com/woocommerce/team-ghidorah/issues/288

ilyasfoo

@chihsuan I'm think the following is caused by this PR, when I ran pnpm run --filter @woocommerce/plugin-woocommerce watch:build, waited it to complete, then make changes to plugins/woocommerce-admin/client/homescreen/index.tsx, it crashes:

../woocommerce-admin watch:build:project:bundle: webpack 5.89.0 compiled successfully in 9223 ms
. watch:build:project:copy-assets:   0% [0 / 1] [0 running]
../woocommerce-admin watch:build:project:bundle: No errors found.
. watch:build:project:copy-assets:   0% [0 / 1] [1 running] build:project:copy-assets
. watch:build:project:copy-assets: ✅ Ran 1 script and skipped 0 in 2.3s.
. watch:build:project:copy-assets: [nodemon] clean exit - waiting for changes before restart
. watch:build:project:copy-assets: [nodemon] restarting due to changes...
. watch:build:project:copy-assets: [nodemon] starting `pnpm run build:project`
. watch:build:project:copy-assets: > @woocommerce/plugin-woocommerce@8.7.0 build:project [redacted]/woocommerce/plugins/woocommerce
. watch:build:project:copy-assets: > pnpm --if-present '/^build:project:.*$/'
../woocommerce-admin watch:build:project:bundle: [webpack-cli] Error: Prevent writing to file that only differs in casing or query string from already written file.
../woocommerce-admin watch:build:project:bundle: This will lead to a race-condition and corrupted files on case-insensitive file systems.
../woocommerce-admin watch:build:project:bundle: [redacted]/woocommerce/plugins/woocommerce-admin/build/chunks/homescreen.js
../woocommerce-admin watch:build:project:bundle: [redacted]/woocommerce/plugins/woocommerce-admin/build/chunks/homescreen.js
../woocommerce-admin watch:build:project:bundle:     at checkSimilarFile ([redacted]/woocommerce/node_modules/.pnpm/webpack@5.89.0_webpack-cli@4.10.0/node_modules/webpack/lib/Compiler.js:666:11)
../woocommerce-admin watch:build:project:bundle:     at writeOut ([redacted]/woocommerce/node_modules/.pnpm/webpack@5.89.0_webpack-cli@4.10.0/node_modules/webpack/lib/Compiler.js:840:13)
../woocommerce-admin watch:build:project:bundle:     at [redacted]/woocommerce/node_modules/.pnpm/webpack@5.89.0_webpack-cli@4.10.0/node_modules/webpack/lib/util/fs.js:242:5
../woocommerce-admin watch:build:project:bundle:     at FSReqCallback.oncomplete (node:fs:196:23)
. watch:build:project:copy-assets: > @woocommerce/plugin-woocommerce@8.7.0 build:project:copy-assets [redacted]/woocommerce/plugins/woocommerce
. watch:build:project:copy-assets: > wireit
../woocommerce-admin watch:build:project:bundle: Error: write EPIPE
../woocommerce-admin watch:build:project:bundle:     at process.target._send (node:internal/child_process:867:20)
../woocommerce-admin watch:build:project:bundle:     at process.target.send (node:internal/child_process:740:19)
../woocommerce-admin watch:build:project:bundle:     at [redacted]/woocommerce/node_modules/.pnpm/fork-ts-checker-webpack-plugin@8.0.0_typescript@5.3.3_webpack@5.89.0/node_modules/fork-ts-checker-webpack-plugin/lib/rpc/expose-rpc.js:27:31
../woocommerce-admin watch:build:project:bundle:     at new Promise (<anonymous>)
../woocommerce-admin watch:build:project:bundle:     at sendMessage ([redacted]/woocommerce/node_modules/.pnpm/fork-ts-checker-webpack-plugin@8.0.0_typescript@5.3.3_webpack@5.89.0/node_modules/fork-ts-checker-webpack-plugin/lib/rpc/expose-rpc.js:19:38)
../woocommerce-admin watch:build:project:bundle:     at [redacted]/woocommerce/node_modules/.pnpm/fork-ts-checker-webpack-plugin@8.0.0_typescript@5.3.3_webpack@5.89.0/node_modules/fork-ts-checker-webpack-plugin/lib/rpc/expose-rpc.js:60:27
../woocommerce-admin watch:build:project:bundle:     at Generator.next (<anonymous>)
../woocommerce-admin watch:build:project:bundle:     at fulfilled ([redacted]/woocommerce/node_modules/.pnpm/fork-ts-checker-webpack-plugin@8.0.0_typescript@5.3.3_webpack@5.89.0/node_modules/fork-ts-checker-webpack-plugin/lib/rpc/expose-rpc.js:5:58) {
../woocommerce-admin watch:build:project:bundle:   errno: -32,
../woocommerce-admin watch:build:project:bundle:   code: 'EPIPE',
../woocommerce-admin watch:build:project:bundle:   syscall: 'write'
../woocommerce-admin watch:build:project:bundle: }
../woocommerce-admin watch:build:project:bundle: ❌ [watch:build:project:bundle] Service exited unexpectedly
../woocommerce-admin watch:build:project:bundle: ❌ 1 script failed.
../woocommerce-admin watch:build:project:bundle: Failed
[redacted]/woocommerce/plugins/woocommerce-admin:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  @woocommerce/admin-library@3.3.0 watch:build:project:bundle: `wireit`
Exit status 1
. watch:build:project:copy-assets: Analyzing
[redacted]/woocommerce/plugins/woocommerce:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  @woocommerce/plugin-woocommerce@8.7.0 watch:build: `pnpm --if-present --workspace-concurrency=Infinity --filter="$npm_package_name..." --parallel '/^watch:build:project:.*$/'`
Exit status 1

chihsuan · 2024-02-23T07:35:29Z

@chihsuan I'm think the following is caused by this PR, when I ran pnpm run --filter @woocommerce/plugin-woocommerce watch:build, waited it to complete, then make changes to plugins/woocommerce-admin/client/homescreen/index.tsx, it crashes:

Good catch! @ilyasfoo It turns out there's a bug in UnminifyWebpackPlugin. We didn't reset the output object, resulting in files with the same name but different query strings being output. Fixed in dfd38e3, even though we're planning to remove it. 😬

ilyasfoo

Works as advertised, but 1 question:

It seems like when having SCRIPT_DEBUG to true, there are some woocommerce assets's version parameter are set to WC version, timestamp, and hash. I'm not sure if this is intended? I wasn't sure what's a good filter for wc-admin's assets.

If so, pre-approving 🚢

ilyasfoo · 2024-02-26T16:06:15Z

plugins/woocommerce/src/Internal/Admin/WCPayPromotion/Init.php

-			WCAdminAssets::get_file_version( 'css' )
-		);
-
-		WCAdminAssets::register_script( 'wp-admin-scripts', 'payment-method-promotions', true );


Good catch!

psealock

@ilyasfoo I believe those are legacy assets which wouldn't be touched by webpack.

@chihsuan, this is testing well 🚀

chihsuan · 2024-02-27T04:42:51Z

It seems like when having SCRIPT_DEBUG to true, there are some woocommerce assets's version parameter are set to WC version, timestamp, and hash. I'm not sure if this is intended? I wasn't sure what's a good filter for wc-admin's assets.

Yes, this is intended. Some are legacy assets as Paul mentioned and some are chunks that can't be enqueued via PHP.

…e asset php for styles

- Use file hash to load scripts/styles ?ver=<file hash> - Add register_style() method to WC_Admin_Assets

…ommerce/product-editor, woocommerce

…s hash (#44838) * Update webpack config to use file content hash for chunks and generate asset php for styles * Use StyleAssetPlugin to generate style.asset.php * Remove unneed ?ver=<version> code * Use file hash from asset file when SCRIPT_DEBUG is off - Use file hash to load scripts/styles ?ver=<file hash> - Add register_style() method to WC_Admin_Assets * Load payment method promotions in admin_enqueue_scripts * Add changefile(s) from automation for the following project(s): @woocommerce/product-editor, woocommerce * Add json2php * Update doc * Update pnpm-lock.yaml * Fix add_print_shipping_label_script * Add a comment to style-asset-plugin.js * Change register_style to use WC_ADMIN_DIST_CSS_FOLDER * Reset the outputNormal object to avoid duplicate files * Fix type error --------- Co-authored-by: github-actions <github-actions@github.com>

github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Feb 21, 2024

chihsuan commented Feb 21, 2024

View reviewed changes

chihsuan self-assigned this Feb 21, 2024

chihsuan closed this Feb 21, 2024

chihsuan reopened this Feb 21, 2024

github-actions bot added the focus: monorepo infrastructure Issues and PRs related to monorepo tooling. label Feb 21, 2024

chihsuan marked this pull request as ready for review February 21, 2024 11:42

chihsuan requested review from a team, psealock and ilyasfoo February 21, 2024 11:42

psealock reviewed Feb 22, 2024

View reviewed changes

chihsuan force-pushed the dev/use-content-hash branch from 9af91a1 to fcdeed1 Compare February 22, 2024 06:12

chihsuan requested a review from psealock February 22, 2024 06:54

ilyasfoo reviewed Feb 22, 2024

View reviewed changes

chihsuan force-pushed the dev/use-content-hash branch from 0c4688d to dfd38e3 Compare February 23, 2024 07:36

chihsuan requested a review from ilyasfoo February 23, 2024 07:36

chihsuan force-pushed the dev/use-content-hash branch 2 times, most recently from aeffba1 to 0a1ea1b Compare February 26, 2024 04:38

ilyasfoo approved these changes Feb 26, 2024

View reviewed changes

psealock approved these changes Feb 27, 2024

View reviewed changes

chihsuan added 4 commits February 27, 2024 13:08

Update webpack config to use file content hash for chunks and generat…

ec0ba74

…e asset php for styles

Use StyleAssetPlugin to generate style.asset.php

d04bb16

Remove unneed ?ver=<version> code

4d66518

Use file hash from asset file when SCRIPT_DEBUG is off

2f63753

- Use file hash to load scripts/styles ?ver=<file hash> - Add register_style() method to WC_Admin_Assets

chihsuan and others added 9 commits February 27, 2024 13:08

Load payment method promotions in admin_enqueue_scripts

76fc585

Add changefile(s) from automation for the following project(s): @wooc…

7c4d57d

…ommerce/product-editor, woocommerce

Add json2php

cad2b69

Update doc

7a2e18d

Update pnpm-lock.yaml

24b150c

Fix add_print_shipping_label_script

e68e6d7

Add a comment to style-asset-plugin.js

4b927f8

Change register_style to use WC_ADMIN_DIST_CSS_FOLDER

8d29407

Reset the outputNormal object to avoid duplicate files

5591fcb

chihsuan force-pushed the dev/use-content-hash branch from 0a1ea1b to 5591fcb Compare February 27, 2024 05:08

Fix type error

595e99f

chihsuan enabled auto-merge (squash) February 27, 2024 05:37

chihsuan merged commit ec8bd31 into trunk Feb 27, 2024
88 of 91 checks passed

chihsuan deleted the dev/use-content-hash branch February 27, 2024 08:07

github-actions bot added this to the 8.8.0 milestone Feb 27, 2024

github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Feb 27, 2024

alopezari added needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve webpack cache-busting version parameter by using file contents hash #44838

Improve webpack cache-busting version parameter by using file contents hash #44838

chihsuan commented Feb 21, 2024 •

edited

chihsuan Feb 21, 2024 •

edited

ilyasfoo Feb 26, 2024

github-actions bot commented Feb 21, 2024 •

edited

github-actions bot commented Feb 21, 2024 •

edited

psealock left a comment

psealock Feb 21, 2024

chihsuan Feb 22, 2024

psealock Feb 21, 2024

psealock Feb 21, 2024

chihsuan Feb 22, 2024

chihsuan Feb 22, 2024

psealock Feb 21, 2024

chihsuan Feb 22, 2024

chihsuan Feb 22, 2024 •

edited

chihsuan commented Feb 22, 2024 •

edited

ilyasfoo left a comment

chihsuan commented Feb 23, 2024 •

edited

ilyasfoo left a comment •

edited

ilyasfoo Feb 26, 2024

psealock left a comment

chihsuan commented Feb 27, 2024

	$this->define( 'WC_ADMIN_DIST_JS_FOLDER', 'assets/client/admin/' );
	$this->define( 'WC_ADMIN_DIST_CSS_FOLDER', 'assets/client/admin/' );

Improve webpack cache-busting version parameter by using file contents hash #44838

Improve webpack cache-busting version parameter by using file contents hash #44838

Conversation

chihsuan commented Feb 21, 2024 • edited

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Changelog entry

Significance

Type

Message

Comment

chihsuan Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2024 • edited

Test Results Summary

github-actions bot commented Feb 21, 2024 • edited

psealock left a 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

chihsuan Feb 22, 2024 • edited

Choose a reason for hiding this comment

chihsuan commented Feb 22, 2024 • edited

ilyasfoo left a comment

Choose a reason for hiding this comment

chihsuan commented Feb 23, 2024 • edited

ilyasfoo left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psealock left a comment

Choose a reason for hiding this comment

chihsuan commented Feb 27, 2024

chihsuan commented Feb 21, 2024 •

edited

chihsuan Feb 21, 2024 •

edited

github-actions bot commented Feb 21, 2024 •

edited

github-actions bot commented Feb 21, 2024 •

edited

chihsuan Feb 22, 2024 •

edited

chihsuan commented Feb 22, 2024 •

edited

chihsuan commented Feb 23, 2024 •

edited

ilyasfoo left a comment •

edited