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.

Sign up for GitHub

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

Jump to bottom

Minify script used for ajax activation of features; warn if absent and serve original file when SCRIPT_DEBUG is enabled #1658

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

Minify script used for ajax activation of features; warn if absent and serve original file when SCRIPT_DEBUG is enabled #1658

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cc3b95f
Add missing JS minification for new PL script
westonruter Nov 17, 2024
1ca7c77
Introduce perflab_get_asset_src()
westonruter Nov 19, 2024
f54c488
Change perflab_get_asset_url() to return path instead of URL
westonruter Nov 19, 2024
be0401c
Eliminate local env check
westonruter Nov 20, 2024
5509f2c
Update other plugins to warn when minified asset is missing
westonruter Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion plugins/performance-lab/includes/admin/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,46 @@ function perflab_dismiss_wp_pointer_wrapper(): void {
}
add_action( 'wp_ajax_dismiss-wp-pointer', 'perflab_dismiss_wp_pointer_wrapper', 0 );

/**
* Gets the path to a script or stylesheet.
*
* @since n.e.x.t
*
* @param string $src_path Source path.
* @param string|null $min_path Minified path. If not supplied, then '.min' is injected before the file extension in the source path.
* @return string URL to script or stylesheet.
*/
function perflab_get_asset_path( string $src_path, ?string $min_path = null ): string {
if ( null === $min_path ) {
// Note: wp_scripts_get_suffix() is not used here because we need access to both the source and minified paths.
$min_path = (string) preg_replace( '/(?=\.\w+$)/', '.min', $src_path );
}

$force_src = false;
if (
( WP_DEBUG || wp_get_environment_type() === 'local' )
Copy link
Member

@felixarntz felixarntz Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense to use WP_DEBUG around this check, because a production build should never be subject to this check (and most production sites don't use WP_DEBUG, at least shouldn't).

I don't see that being used around the checks in our other plugins like Embed Optimizer, Image Prioritizer, and Optimization Detective though. Those should also check this only if WP_DEBUG is enabled. I understand for those having a non-built asset is more critical, but that matters just as little for production builds of them as it does for a production build of PL here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I included the 'local' check because just because you're on a non-production environment doesn't mean that you're going to have WP_DEBUG enabled. Sites spun up with Local do not have WP_DEBUG enabled. Also, even wp-env does not have WP_DEBUG enabled in the test environment (see #1271). So I was intending to increase the opportunities for the issue to be detected. That said, if WP_DEBUG is not enabled, then wp_trigger_error() doesn't do anything, so the result would be that the minified script would continue to be served even though it would result in a 404 (e.g. on production).

The 'local' check isn't in the other plugins I was intending to roll out this check to the other plugins once reviewed here. But I'm less convinced now. I'll remove the 'local' env check.

Copy link
Member

Choose a reason for hiding this comment

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

My point in this comment was mostly that I think we should also add a WP_DEBUG check before the checks in those plugins, because it shouldn't be needed for production builds of the plugin anyway. Something for another PR of course, but I think that would be good to add.

It's not only about whether wp_trigger_error() actually triggers the error, but also about the check itself. Since it does nothing when WP_DEBUG is disabled, that's arguably an even better reason to wrap the check with WP_DEBUG as well.

&&
! file_exists( PERFLAB_PLUGIN_DIR_PATH . $min_path )
Copy link
Member

Choose a reason for hiding this comment

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

So this means a regular (non WP_DEBUG) site would not have the presence of the file checked right? That makes sense to me because in the built plugin it would always exist, and it avoids a file_exists() check we don't need in that situation.

That said, I don't think we should use wp_get_environment_type() here. WP_DEBUG alone should be sufficient as a differentiator. We don't know whether a local environment is used to work on this plugin or just a general site where the built version of this plugin is installed - in the latter case there's no difference from a production environment for what we're doing here.

) {
$force_src = true;
wp_trigger_error(
Copy link
Member

Choose a reason for hiding this comment

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

If someone get these Warning: perflab_get_asset_path(): Minified asset has not been built: includes/admin/plugin-activate-ajax.min.js in /var/www/html/wp-includes/functions.php on line 6114 error in production they didn't understand how to solve these or from which plugin the error comes.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally this shouldn't be checked in a production build of the plugin because there those files will always exist, so it is indeed only for development of the plugin.

Using WP_DEBUG is not an ideal solution because it doesn't give any indication for how this plugin is being used (e.g. in development version like git checkout, or production build). That said, a better way to indicate that may involve some extra work that I'm not sure would be worth it.

cc @westonruter

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it would be extremely unlikely that someone would install the git repo directly onto production. The purpose of this code here is exclusively targeting plugin contributors who are cloning the repo anew or checking out a new branch in which a new asset appears. This error will inform them of the need to do a build while at the same time falling back to the source file rather than the minified file. And when WP_DEBUG is enabled (as it normally should be during development) then the user will get the notices. If someone has SCRIPT_DEBUG enabled, they'll get the source versions anyway, but if WP_DEBUG is enabled it will still do the checks to see if the minified file is present as this indicates they need to do a build.

__FUNCTION__,
sprintf(
/* translators: %s is the minified asset path */
__( 'Minified asset has not been built: %s', 'performance-lab' ),
$min_path
),
E_USER_WARNING
);
}

if ( SCRIPT_DEBUG || $force_src ) {
return $src_path;
}

return $min_path;
}

/**
* Callback function to handle admin scripts.
*
Expand All @@ -228,7 +268,7 @@ function perflab_enqueue_features_page_scripts(): void {
// Enqueue plugin activate AJAX script and localize script data.
wp_enqueue_script(
'perflab-plugin-activate-ajax',
plugin_dir_url( PERFLAB_MAIN_FILE ) . 'includes/admin/plugin-activate-ajax' . wp_scripts_get_suffix() . '.js',
plugin_dir_url( PERFLAB_MAIN_FILE ) . perflab_get_asset_path( 'includes/admin/plugin-activate-ajax.js' ),
array( 'wp-i18n', 'wp-a11y', 'wp-api-fetch' ),
PERFLAB_VERSION,
true
Expand Down