Update Optimization Detective plugin build process #1080

thelovekesh · 2024-03-22T09:04:58Z

Summary

github-actions · 2024-03-22T09:05:16Z

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: thelovekesh <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: felixarntz <[email protected]>

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

thelovekesh · 2024-03-22T09:55:03Z

@westonruter @felixarntz I have updated the build steps.

With this, I would suggest separating the standalone plugins' building process from the primary PL plugin. I recommend this for the following reasons:

The size of the main webpack file and thus maintenance overhead will increase as more plugins employ build steps.
It will be difficult to share the sharedConfig across all plugins and build phases if any plugin expands the shared configuration.
Handling the dependencies option will get challenging.

In 767c895, I have made the necessary changes. LMKWYT and I'll make the necessary adjustments.

westonruter · 2024-03-22T13:56:14Z

@thelovekesh Sounds good to me but I'm a bit confused: are you saying you did do it already or have yet to do it?

westonruter · 2024-03-22T13:53:16Z

package.json

@@ -28,11 +28,14 @@
    "changelog": "./bin/plugin/cli.js changelog",
    "since": "./bin/plugin/cli.js since",
    "readme": "./bin/plugin/cli.js readme",
-    "build": "wp-scripts build",
+    "build": "npm-run-all 'build:!(plugin)'",
+    "build:optimization-detective": "wp-scripts build -c plugins/optimization-detective/webpack.config.js",


The standalone plugin deploy workflow is just doing:

npm run build:plugin:${{ matrix.plugin }}

So won't this mean that the assets aren't included in the plugin build there?

Separately, in Optimization Detective, when first cloning the repo for development, isn't there a check to see if the web-vitals script is present and if not an admin notice is shown with instructions to build the plugin? Won't that need to be updated?

Will this be getting run automatically when doi

It will be run automatically on the pre-hook. See diff.

Oh I see (and sorry for my truncated com

😉

Separately, in Optimization Detective, when first cloning the repo for development, isn't there a check to see if the web-vitals script is present, and if not an admin notice is shown with instructions to build the plugin? Won't that need to be updated?

I think we did it in the can-load.php file which no longer exists. We need to add this check in the main plugin file and die with an admin notice if the required assets are not there. Shall I add this as the part of same PR?

Oh right. If we think it's important to add, or alternatively what if the asset build happens automatically after npm install?

Yeah, they would get a fatal error on the frontend when attempting to require the asset PHP file. In PHP7 can't this Error be caught? If so, we could then show an error in the console about needing to build.

How about this:

--- a/plugins/optimization-detective/detection.php +++ b/plugins/optimization-detective/detection.php @@ -35,8 +35,17 @@ function od_get_detection_script( string $slug, OD_URL_Metrics_Group_Collection */ $detection_time_window = apply_filters( 'od_detection_time_window', 5000 ); - $web_vitals_lib_data = require __DIR__ . '/detection/web-vitals.asset.php'; - $web_vitals_lib_src = add_query_arg( 'ver', $web_vitals_lib_data['version'], plugin_dir_url( __FILE__ ) . '/detection/web-vitals.js' ); + try { + $web_vitals_lib_data = require __DIR__ . '/detection/web-vitals.asset.php'; + } catch ( Error $error ) { + return wp_get_inline_script_tag( + sprintf( + 'console.error( %s );', + wp_json_encode( '[Optimization Detective] ' . __( 'Unable to load web-vitals.asset.php. Please make sure you have run `npm install && npm run build:optimization-detective`.', 'optimization-detective' ) ) + ) + ); + } + $web_vitals_lib_src = add_query_arg( 'ver', $web_vitals_lib_data['version'], plugin_dir_url( __FILE__ ) . '/detection/web-vitals.js' ); $current_url = od_get_current_url(); $detect_args = array(

If we are going to add it, I think the plugin screen would be the best place as users will come to know about this as soon as the plugin is activated. But above approach also looks good to me.

Added in 4656cbf

See my other comment: This NPM script is confusing, why not put it directly into prebuild:plugin:optimization-detective instead?

thelovekesh · 2024-03-22T14:15:47Z

@thelovekesh Sounds good to me but I'm a bit confused: are you saying you did do it already or have yet to do it?

I have done it in a single commit(767c895) for others to test it and to revert if required.

westonruter · 2024-03-22T16:54:12Z

.github/workflows/deploy-standalone-plugins.yml

-      - name: Setup Node.js (.nvmrc)
-        uses: actions/setup-node@v3
-        with:
-          node-version-file: '.nvmrc'
-          cache: npm
-      - name: Install npm dependencies
-        run: npm ci
-      - name: Build assets
-        run: npm run build
-      - name: Get directory
-        id: get-plugin-directory
-        if: ${{ github.event_name == 'workflow_dispatch' }}
-        run: |
-          echo "directory=$(node ./bin/plugin/cli.js get-plugin-dir --slug=${{ inputs.slug }})" >> $GITHUB_OUTPUT
-      - name: Get plugin version
-        id: get-version
-        if: ${{ github.event_name == 'workflow_dispatch' }}
-        run: |
-          echo "version=$(node ./bin/plugin/cli.js get-plugin-version --slug=${{ inputs.slug }})" >> $GITHUB_OUTPUT


I see this is removed because it is duplicated with the deploy job below.

felixarntz

@thelovekesh A bit of feedback below. Let's try to limit the build workflow changes in the feature branch to things that only affect this plugin. Otherwise it should be explored in a PR against trunk, to avoid siloed decisions and decrease the risk of merge conflicts in the future.

felixarntz · 2024-03-22T17:55:01Z

package.json

+    "prebuild:plugin:optimization-detective": "npm run build:optimization-detective",
    "build:plugin:optimization-detective": "webpack --mode production --env plugin=optimization-detective",


I find this very confusing. Why is there build:plugin:optimization-detective and build:optimization-detective, the latter of which is then called by prebuild:plugin:optimization-detective? Can we simplify this?

It's not obvious how build:plugin:optimization-detective and build:optimization-detective differ. I think one of them should be removed and implemented in a more low-level fashion that doesn't lead to confusion about the available scripts.

felixarntz · 2024-03-22T17:57:18Z

package.json

@@ -28,11 +28,14 @@
    "changelog": "./bin/plugin/cli.js changelog",
    "since": "./bin/plugin/cli.js since",
    "readme": "./bin/plugin/cli.js readme",
-    "build": "wp-scripts build",
+    "build": "npm-run-all 'build:!(plugin)'",
+    "build:optimization-detective": "wp-scripts build -c plugins/optimization-detective/webpack.config.js",


See my other comment: This NPM script is confusing, why not put it directly into prebuild:plugin:optimization-detective instead?

felixarntz · 2024-03-22T17:59:02Z

package.json

-    "build": "wp-scripts build",
+    "build": "npm-run-all 'build:!(plugin)'",


What's the purpose of this? Why not use wp-scripts build, which is intended to take away a lot of the maintenance burden of custom build workflows? Are there concrete limitations that make using it infeasible?

felixarntz · 2024-03-22T18:01:20Z

plugins/optimization-detective/detection.php

+	try {
+		$web_vitals_lib_data = require __DIR__ . '/detection/web-vitals.asset.php';
+	} catch ( Error $error ) {
+		$error_message  = '[Optimization Detective] ';
+		$error_message .= sprintf(
+			/* translators: 1: File path. 2: CLI command. */
+			__( 'Unable to load %1$s. Please make sure you have run %2$s.', 'optimization-detective' ),
+			'detection/web-vitals.asset.php',
+			'`npm install && npm run build:optimization-detective`'
+		);
+		if ( WP_DEBUG && WP_DEBUG_DISPLAY ) {
+			$error_message .= ' ' . __( 'PHP Error:', 'optimization-detective' ) . ' ' . $error->getMessage();
+		}
+		return wp_get_inline_script_tag(
+			sprintf( 'console.error( %s );', wp_json_encode( $error_message ) )
+		);
+	}
+	$web_vitals_lib_src = add_query_arg( 'ver', $web_vitals_lib_data['version'], plugin_dir_url( __FILE__ ) . '/detection/web-vitals.js' );


Shouldn't this rather be a check in the plugin main file? If that file is missing, the plugin needs to be built, just how e.g. other plugins commonly check for existence of composer.json or vendor dir. The lack of this should be obvious to the end user, IMO better in an admin notice that shows immediately rather than a hard-to-detect PHP error that is only shown in JavaScript and only if the detection script is attempted to be loaded.

This is the only place where the file is actually accessed. Also, the check is only relevant during development. It seems wasteful to do a file_exists() check with every request in production when in a production build the files are always present.

Also, the check is only relevant during development.

So is checking for things like composer.json or vendor, but it's commonly included in many plugins' main file. My main feedback here is that I don't see why this file missing should not be treated in the same way as such cases. It's needed for the plugin to function correctly, as far as I understand.

OK. Reverted in 6ae5fb0

Moved to main file in 3aae425

felixarntz · 2024-03-22T18:05:09Z

plugins/optimization-detective/webpack.config.js

+	};
+};
+
+module.exports = optimizationDetective;


Which parts of this file are actually needed?

Background is I don't recall that we discussed having individual webpack scripts per plugin before, and since the only plugin that needs a build process to function at all so far is this one, I think we should go with a more pragmatic solution. Why not include this Optimization Detective specific web-vitals step in the main webpack.config.js? In other words, the way this was before.

We can consider optimizing it later, let's just get to an MVP solution now. Decisions that affect the build infrastructure of the entire repo shouldn't be made in a plugin's feature branch PR.

Per #1080 (comment) it seems like you're saying 767c895 should be reverted?

@westonruter Yes, I think that's a much simpler solution for now, especially since this is the only plugin that needs a build process.

Reverted in 0e07d11

This reverts commit 4656cbf.

This reverts commit 767c895.

thelovekesh · 2024-03-22T18:58:36Z

plugins/optimization-detective/load.php

@@ -25,6 +25,21 @@
 	return;
 }

+if ( ! file_exists( __DIR__ . '/detection/web-vitals.asset.php' ) ) {


@westonruter Can't we wrap it in is_admin()? Users will face this problem while activating the plugin. So having this in admin should suffice? In production this check will be redundant anyway.

Good idea! Did that in ae07548 as well as checking if WP-CLI.

@westonruter @thelovekesh Alternatively we could consider using require in a try catch like you had before, given you mentioned the file_exists check as a potential concern.

I think this is good now as otherwise it would need to require the PHP file on every request when it may not be needed.

westonruter · 2024-03-22T19:13:53Z

Note: the PL plugin unit tests are currently failing because wp-env is activating all standalone plugins in the workflow, even though they haven't been built (and so Optimization Detective is rightfully throwing an error since the web-vitals.js is absent).

So to fix this we can update the workflow to edit the .wp-env.json to remove all standalone plugins from the plugins key, so it remains just ["."]:

diff --git a/.wp-env.json b/.wp-env.json
index be6a3114..c59e89af 100644
--- a/.wp-env.json
+++ b/.wp-env.json
@@ -1,14 +1,6 @@
 {
   "core": null,
-  "plugins": [
-    ".",
-    "./plugins/auto-sizes",
-    "./plugins/dominant-color-images",
-    "./plugins/embed-optimizer",
-    "./plugins/optimization-detective",
-    "./plugins/speculation-rules",
-    "./plugins/webp-uploads"
-  ],
+  "plugins": ["."],
   "env": {
     "tests": {
       "config": {

And as discussed with @thelovekesh, this file can be written to .wp-env.override.json before proceeding with npm run wp-env start.

…env config

felixarntz

LGTM, just one follow up.

felixarntz · 2024-03-22T19:26:32Z

.github/workflows/php-test.yml

+      - name: Remove standalone plugins from wp-env config
+        run: jq '.plugins = [.plugins[0]]' .wp-env.json > .wp-env.override.json


felixarntz · 2024-03-22T19:28:47Z

plugins/optimization-detective/load.php

+	! file_exists( __DIR__ . '/detection/web-vitals.asset.php' )
+) {
+	// phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error
+	trigger_error(


Why not use wp_trigger_error()? Or even more obvious with an admin notice, like for instance in https://github.com/ampproject/amp-wp/blob/develop/amp.php#L178?

The problem with wp_trigger_error() is that it does nothing if WP_DEBUG isn't enabled (which seems somewhat strange especially if E_USER_ERROR had been passed).

As for a more robust error notices like in the AMP plugin, I think that might be overkill for Optimization Detective. There are a lot more dependencies for the AMP plugin which can cause errors to happen a lot more often, so more users will be seeing that notice, even end users. For Optimization Detective, the only users seeing the error would be developers cloning from GitHub. So I think what we have now is sufficient.

Makes sense regarding wp_trigger_error(). Or rather not - well...

Regarding the AMP plugin, as to what I had mentioned before, there's also https://github.com/ampproject/amp-wp/blob/develop/amp.php#L162 which shows an admin notice for when the plugin isn't built. That's just as much applicable here, which is why I was proposing it.

But not a necessity to have either way.

Yeah, for that we just included it among the other load error checks since the infrastructure was already there. But since that is a build time error rather than a runtime error, it technically isn't needed for the production build.

thelovekesh added 6 commits March 22, 2024 10:11

Remove redundant build steps from workflows

d5785eb

Move utils from webpack config to bin/webpack

381ba0f

Update optimization-detective plugin build step

c2815e3

Move webpack utils import to Internal dependencies

d0cb266

Add getPluginRootPath() to exports

2f3605a

Add build:plugin command for embed-optimizer plugin

b246546

thelovekesh requested review from mukeshpanchal27 and felixarntz as code owners March 22, 2024 09:04

thelovekesh added Infrastructure Issues for the overall performance plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature no milestone PRs that do not have a defined milestone for release labels Mar 22, 2024

thelovekesh requested a review from westonruter as a code owner March 22, 2024 09:25

Decouple plugins asset building from main Webpack config

767c895

thelovekesh force-pushed the update/build-process branch from 2118490 to 767c895 Compare March 22, 2024 09:53

westonruter reviewed Mar 22, 2024

View reviewed changes

westonruter changed the title ~~Update Optimization Detective plguin build process~~ Update Optimization Detective plugin build process Mar 22, 2024

westonruter mentioned this pull request Mar 22, 2024

Merge Optimization Detective into trunk #1079

Merged

2 tasks

Add console error when web-vitals.js is absent

4656cbf

westonruter reviewed Mar 22, 2024

View reviewed changes

westonruter approved these changes Mar 22, 2024

View reviewed changes

felixarntz requested changes Mar 22, 2024

View reviewed changes

westonruter added 3 commits March 22, 2024 11:31

Revert "Add console error when web-vitals.js is absent"

6ae5fb0

This reverts commit 4656cbf.

Revert "Decouple plugins asset building from main Webpack config"

0e07d11

This reverts commit 767c895.

Add web-vitals file exists check to load.php

3aae425

westonruter requested a review from felixarntz March 22, 2024 18:54

thelovekesh commented Mar 22, 2024

View reviewed changes

Only do file exists check if in admin or WP-CLI

ae07548

Update PL plugin tests workflow to remove standlaone plugins from wp-…

bc906ae

…env config

felixarntz approved these changes Mar 22, 2024

View reviewed changes

westonruter merged commit 8d096f1 into feature/image-loading-optimization Mar 22, 2024
32 checks passed

westonruter deleted the update/build-process branch March 22, 2024 20:05

		"prebuild:plugin:optimization-detective": "npm run build:optimization-detective",
		"build:plugin:optimization-detective": "webpack --mode production --env plugin=optimization-detective",

		"build": "wp-scripts build",
		"build": "npm-run-all 'build:!(plugin)'",

		- name: Remove standalone plugins from wp-env config
		run: jq '.plugins = [.plugins[0]]' .wp-env.json > .wp-env.override.json

Update Optimization Detective plugin build process #1080

Update Optimization Detective plugin build process #1080

Conversation

thelovekesh commented Mar 22, 2024

Summary

github-actions bot commented Mar 22, 2024 • edited Loading

thelovekesh commented Mar 22, 2024

westonruter commented Mar 22, 2024

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

westonruter Mar 22, 2024 • edited Loading

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

thelovekesh commented Mar 22, 2024

westonruter Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

felixarntz 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

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

westonruter commented Mar 22, 2024

felixarntz 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

github-actions bot commented Mar 22, 2024 •

edited

Loading

westonruter Mar 22, 2024 •

edited

Loading

westonruter Mar 22, 2024 •

edited

Loading