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

Update Optimization Detective plugin build process #1080

Merged
merged 13 commits into from
Mar 22, 2024
Merged
Changes from 7 commits
Commits
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
11 changes: 2 additions & 9 deletions .github/workflows/deploy-dotorg.yml
Original file line number Diff line number Diff line change
@@ -13,21 +13,14 @@ jobs:
steps:
- name: Checkout
uses: actions/checkout@v4
- name: Setup Node.js (.nvmrc)
uses: actions/setup-node@v3
with:
node-version-file: ".nvmrc"
cache: npm
- name: npm install
run: npm ci
- name: Build assets
run: npm run build

- name: WordPress plugin deploy
uses: 10up/action-wordpress-plugin-deploy@stable
env:
SVN_PASSWORD: ${{ secrets.SVN_PASSWORD }}
SVN_USERNAME: ${{ secrets.SVN_USERNAME }}
SLUG: performance-lab

- name: Upload release assets
uses: softprops/action-gh-release@v1
if: startsWith(github.ref, 'refs/tags/')
20 changes: 1 addition & 19 deletions .github/workflows/deploy-standalone-plugins.yml
Original file line number Diff line number Diff line change
@@ -27,25 +27,7 @@ jobs:
steps:
- name: Checkout
uses: actions/checkout@v4
- 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
Comment on lines -30 to -48
Copy link
Member

@westonruter westonruter Mar 22, 2024

Choose a reason for hiding this comment

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

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


- name: Set matrix
id: set-matrix
run: |
2 changes: 0 additions & 2 deletions .github/workflows/php-test.yml
Original file line number Diff line number Diff line change
@@ -62,8 +62,6 @@ jobs:
cache: npm
- name: npm install
run: npm ci
- name: Build assets
run: npm run build
- name: Install WordPress
run: npm run wp-env start
# Note that `composer update` is required instead of `composer install`
22 changes: 22 additions & 0 deletions bin/webpack/utils.js
Original file line number Diff line number Diff line change
@@ -77,8 +77,30 @@ const generateBuildManifest = ( slug, from ) => {
fs.writeFileSync( manifestPath, JSON.stringify( manifest, null, 2 ) );
};

/**
* Transformer to get version from package.json and return it as a PHP file.
*
* @param {Buffer} content The content as a Buffer of the file being transformed.
* @param {string} absoluteFrom The absolute path to the file being transformed.
*
* @return {Buffer|string} The transformed content.
*/
const assetDataTransformer = ( content, absoluteFrom ) => {
if ( 'package.json' !== path.basename( absoluteFrom ) ) {
return content;
}

const contentAsString = content.toString();
const contentAsJson = JSON.parse( contentAsString );
const { version } = contentAsJson;

return `<?php return array('dependencies' => array(), 'version' => '${ version }');`;
};

module.exports = {
getPluginRootPath,
deleteFileOrDirectory,
getPluginVersion,
generateBuildManifest,
assetDataTransformer,
};
5 changes: 4 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -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)'",
Copy link
Member

Choose a reason for hiding this comment

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

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?

"build:optimization-detective": "wp-scripts build -c plugins/optimization-detective/webpack.config.js",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see (and sorry for my truncated com

😉

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@westonruter westonruter Mar 22, 2024

Choose a reason for hiding this comment

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

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(

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Added in 4656cbf

Copy link
Member

Choose a reason for hiding this comment

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

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

"build-plugins": "npm-run-all 'build:plugin:!(performance-lab)'",
"build:plugin:performance-lab": "rm -rf build/performance-lab && mkdir -p build/performance-lab && git archive HEAD | tar -x -C build/performance-lab",
"build:plugin:auto-sizes": "webpack --mode production --env plugin=auto-sizes",
"build:plugin:dominant-color-images": "webpack --mode production --env plugin=dominant-color-images",
"build:plugin:embed-optimizer": "webpack --mode production --env plugin=embed-optimizer",
"prebuild:plugin:optimization-detective": "npm run build:optimization-detective",
"build:plugin:optimization-detective": "webpack --mode production --env plugin=optimization-detective",
Copy link
Member

Choose a reason for hiding this comment

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

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.

"build:plugin:speculation-rules": "webpack --mode production --env plugin=speculation-rules",
"build:plugin:webp-uploads": "webpack --mode production --env plugin=webp-uploads",
78 changes: 78 additions & 0 deletions plugins/optimization-detective/webpack.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/**
* External dependencies
*/
const path = require( 'path' );
const WebpackBar = require( 'webpackbar' );
const CopyWebpackPlugin = require( 'copy-webpack-plugin' );

/**
* Internal dependencies
*/
const {
getPluginRootPath,
assetDataTransformer,
} = require( '../../bin/webpack/utils' );

/**
* WordPress dependencies
*/
const defaultConfig = require( '@wordpress/scripts/config/webpack.config' );

const sharedConfig = {
...defaultConfig,
entry: {},
output: {},
};

/**
* Webpack Config: Optimization Detective
*
* @param {*} env Webpack environment
* @return {Object} Webpack configuration
*/
const optimizationDetective = ( env ) => {
if ( env.plugin && env.plugin !== 'optimization-detective' ) {
return {
entry: {},
output: {},
};
}

const source = path.resolve(
getPluginRootPath(),
'node_modules/web-vitals'
);
const destination = path.resolve(
getPluginRootPath(),
'plugins/optimization-detective/detection'
);

return {
...sharedConfig,
name: 'optimization-detective',
plugins: [
new CopyWebpackPlugin( {
patterns: [
{
from: `${ source }/dist/web-vitals.js`,
to: `${ destination }/web-vitals.js`,
},
{
from: `${ source }/package.json`,
to: `${ destination }/web-vitals.asset.php`,
transform: {
transformer: assetDataTransformer,
cache: false,
},
},
],
} ),
new WebpackBar( {
name: 'Building Optimization Detective Assets',
color: '#2196f3',
} ),
],
};
};

module.exports = optimizationDetective;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Reverted in 0e07d11

72 changes: 8 additions & 64 deletions webpack.config.js
Original file line number Diff line number Diff line change
@@ -4,15 +4,15 @@
const path = require( 'path' );
const WebpackBar = require( 'webpackbar' );
const CopyWebpackPlugin = require( 'copy-webpack-plugin' );
const {
deleteFileOrDirectory,
generateBuildManifest,
} = require( './bin/webpack/utils' );

/**
* Internal dependencies
*/
const { plugins: standalonePlugins } = require( './plugins.json' );
const {
deleteFileOrDirectory,
generateBuildManifest,
} = require( './bin/webpack/utils' );

/**
* WordPress dependencies
@@ -25,60 +25,6 @@ const sharedConfig = {
output: {},
};

/**
* Transformer to get version from package.json and return it as a PHP file.
*
* @param {Buffer} content The content as a Buffer of the file being transformed.
* @param {string} absoluteFrom The absolute path to the file being transformed.
*
* @return {Buffer|string} The transformed content.
*/
const assetDataTransformer = ( content, absoluteFrom ) => {
if ( 'package.json' !== path.basename( absoluteFrom ) ) {
return content;
}

const contentAsString = content.toString();
const contentAsJson = JSON.parse( contentAsString );
const { version } = contentAsJson;

return `<?php return array('dependencies' => array(), 'version' => '${ version }');`;
};

const webVitals = () => {
const source = path.resolve( __dirname, 'node_modules/web-vitals' );
const destination = path.resolve(
__dirname,
'plugins/optimization-detective/detection'
);

return {
...sharedConfig,
plugins: [
new CopyWebpackPlugin( {
patterns: [
{
from: `${ source }/dist/web-vitals.js`,
to: `${ destination }/web-vitals.js`,
},
{
from: `${ source }/package.json`,
to: `${ destination }/web-vitals.asset.php`,
transform: {
transformer: assetDataTransformer,
cache: false,
},
},
],
} ),
new WebpackBar( {
name: 'Web Vitals',
color: '#f5a623',
} ),
],
};
};

/**
* Webpack configuration for building the plugin for distribution.
* Note: Need to pass plugin name like `--env.plugin=plugin-name` to build particular plugin.
@@ -119,9 +65,10 @@ const buildPlugin = ( env ) => {
globOptions: {
dot: true,
ignore: [
'**/*.[Cc]ache',
'**/.wordpress-org',
'**/phpcs.xml.dist',
'**/*.[Cc]ache',
'**/webpack.config.js',
],
},
},
@@ -141,14 +88,11 @@ const buildPlugin = ( env ) => {
},
},
new WebpackBar( {
name: `Building ${ env.plugin }`,
name: `Building ${ env.plugin } Plugin`,
color: '#4caf50',
} ),
],
dependencies: [
// Add any dependencies here which should be built before the plugin.
],
};
};

module.exports = [ webVitals, buildPlugin ];
module.exports = [ buildPlugin ];