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

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

Merged
merged 5 commits into from
Nov 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 1 addition & 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 @@ -228,7 +228,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.js',
plugin_dir_url( PERFLAB_MAIN_FILE ) . 'includes/admin/plugin-activate-ajax' . wp_scripts_get_suffix() . '.js',
Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Member

@felixarntz felixarntz Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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

array( 'wp-i18n', 'wp-a11y', 'wp-api-fetch' ),
PERFLAB_VERSION,
true
Expand Down
35 changes: 35 additions & 0 deletions webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,46 @@ const sharedConfig = {

// Store plugins that require build process.
const pluginsWithBuild = [
'performance-lab',
'embed-optimizer',
'image-prioritizer',
'optimization-detective',
'web-worker-offloading',
];

/**
* Webpack Config: Performance Lab
*
* @param {*} env Webpack environment
* @return {Object} Webpack configuration
*/
const performanceLab = ( env ) => {
if ( env.plugin && env.plugin !== 'performance-lab' ) {
return defaultBuildConfig;
}

const pluginDir = path.resolve( __dirname, 'plugins/performance-lab' );

return {
...sharedConfig,
name: 'performance-lab',
plugins: [
new CopyWebpackPlugin( {
patterns: [
{
from: `${ pluginDir }/includes/admin/plugin-activate-ajax.js`,
to: `${ pluginDir }/includes/admin/plugin-activate-ajax.min.js`,
},
],
} ),
new WebpackBar( {
name: 'Building Performance Lab Assets',
color: '#2196f3',
} ),
],
};
};

/**
* Webpack Config: Embed Optimizer
*
Expand Down Expand Up @@ -286,6 +320,7 @@ const buildPlugin = ( env ) => {
};

module.exports = [
performanceLab,
embedOptimizer,
imagePrioritizer,
optimizationDetective,
Expand Down