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

Correct version handling for precached assets #317

Merged
merged 3 commits into from
Aug 28, 2020

Conversation

ethitter
Copy link
Contributor

Investigating cache misses for precached assets revealed that the plugin wasn't building URLs in the same way that the WP_Scripts and WP_Styles classes do.

The revision handling also contributed to cache misses. Per https://developers.google.com/web/tools/workbox/modules/workbox-precaching#explanation_of_the_precache_list, when the URL includes a version, the revision should be set to null.

Per https://developers.google.com/web/tools/workbox/modules/workbox-precaching#explanation_of_the_precache_list,
when the URL includes a version, the revision should be set to `null`.

While investigating cache misses for precached assets, a discrepancy was found between
how the plugin handles asset versions and how WordPress uses them when enqueueing
those items.
@westonruter
Copy link
Collaborator

Good catch.

In looking at WP_Scripts::do_item() I can see something else that may be missing (same in WP_Styles::do_item()):

if ( isset( $this->args[ $handle ] ) ) {
	$ver = $ver ? $ver . '&' . $this->args[ $handle ] : $this->args[ $handle ];
}

I've never actually seen this code executed before. I'm not sure it's even used in core. Nevertheless, I can see that Jetpack is also accounting for this code in how it reconstructs script URLs.

@ethitter
Copy link
Contributor Author

Amusingly, I added that to Infinite Scroll. I don't think I've ever seen it used, but if the handle is passed as foo?bar, then bar is added to the query string. I'll update the PR to account for that.

Copy link
Collaborator

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this next week.

@westonruter westonruter added this to the 0.6 milestone Aug 28, 2020
@westonruter westonruter merged commit 67515a7 into GoogleChromeLabs:develop Aug 28, 2020
Copy link
Collaborator

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ethitter One thing I realized after merging…

}

if ( isset( wp_styles()->args[ $handle ] ) ) {
$version = $version ? $version . '&' . wp_styles()->args[ $handle ] : wp_styles()->args[ $handle ];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto for above:

Suggested change
$version = $version ? $version . '&' . wp_styles()->args[ $handle ] : wp_styles()->args[ $handle ];
$version = $version ? $version . '&' . wp_styles()->args[ $handle ] : wp_styles()->args[ $handle ];

}

if ( isset( wp_scripts()->args[ $handle ] ) ) {
$version = $version ? $version . '&' . wp_scripts()->args[ $handle ] : wp_scripts()->args[ $handle ];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that in the context of the service worker, the & here should be & since the resulting URL will not be output to HTML for the browser to decode & back to &:

Suggested change
$version = $version ? $version . '&' . wp_scripts()->args[ $handle ] : wp_scripts()->args[ $handle ];
$version = $version ? $version . '&' . wp_scripts()->args[ $handle ] : wp_scripts()->args[ $handle ];

@westonruter
Copy link
Collaborator

@ethitter Please review #325

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

Successfully merging this pull request may close these issues.

2 participants