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

[westonruter]

This is a follow-up to #1643 and #1646.

This adds minification of the plugin-activate-ajax.js file that was introduced in #1646, and it ensures the non-minified version is also distributed according to #1643.

This is not a critical PR to include in the 3.6.0 release since the JS file in question is small and it's only served in the admin.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[felixarntz]

With this added, we should probably add a warning similar to how the other plugins with build processes have? To check in WP Admin that that file actually exists, and otherwise warn about having to run the build.

[westonruter]

I thought about that, but it's unfortunate that the path is hard-coded for each plugin's unique JS file. For Optimization Detective and Web Worker Offloading this build step is critical because the libs for web-vitals and partytown need to be included. Those plugins should be checking specifically for those libs to be present.

For all other cases, however, there is no need for the minified scripts. They could instead just reference the non-minified file instead.

So what about this: if SCRIPT_DEBUG is not enabled, we check if the minified script is available, and if not, we force to use the non-minified version and emit a wp_trigger_error() saying that a build is required. A helper function could be copied into each plugin which does this.

[felixarntz]

I think that makes sense. Agree we don't need to be as "aggressive" about warning about the build process here, but I think we need to avoid unconditionally calling wp_scripts_get_suffix() if we don't know whether the minified version is there.

[westonruter]

Here's what I have in mind: https://github.com/WordPress/performance/pull/1658/files/cc3b95fdeaab99efeeba964fbee2c5bd589680ce..f54c4884215f6691d934462ca5aedd9820f20d9e

This same function can then be copied into each PL plugin that needs to load a script or stylesheet, and the error will be emitted for each script/stylesheet referenced instead of arbitrarily checking that just one has been built (which would be problematic when additional JS/CSS files are added and then a dev could be confused why it isn't working).

[felixarntz]

Pretty much, that looks like a good idea. I left comments on that change.

[felixarntz]

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.

[felixarntz]

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.

[westonruter]

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.

[felixarntz]

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.

[mukeshpanchal27]

Q. Do we add these logic for development purpose only? As if .min is not available then it load full version with the notice.

[mukeshpanchal27]

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.

[felixarntz]

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

[westonruter]

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.