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

Update Optimization Detective plugin build process #1080

Merged
merged 13 commits into from
Mar 22, 2024
Merged

Update Optimization Detective plugin build process #1080

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
d5785eb
Remove redundant build steps from workflows
thelovekesh Mar 22, 2024
381ba0f
Move utils from webpack config to bin/webpack
thelovekesh Mar 22, 2024
c2815e3
Update optimization-detective plugin build step
thelovekesh Mar 22, 2024
d0cb266
Move webpack utils import to Internal dependencies
thelovekesh Mar 22, 2024
2f3605a
Add getPluginRootPath() to exports
thelovekesh Mar 22, 2024
b246546
Add build:plugin command for embed-optimizer plugin
thelovekesh Mar 22, 2024
767c895
Decouple plugins asset building from main Webpack config
thelovekesh Mar 22, 2024
4656cbf
Add console error when web-vitals.js is absent
westonruter Mar 22, 2024
6ae5fb0
Revert "Add console error when web-vitals.js is absent"
westonruter Mar 22, 2024
0e07d11
Revert "Decouple plugins asset building from main Webpack config"
westonruter Mar 22, 2024
3aae425
Add web-vitals file exists check to load.php
westonruter Mar 22, 2024
ae07548
Only do file exists check if in admin or WP-CLI
westonruter Mar 22, 2024
bc906ae
Update PL plugin tests workflow to remove standlaone plugins from wp-…
thelovekesh Mar 22, 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
15 changes: 15 additions & 0 deletions plugins/optimization-detective/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,21 @@
return;
}

if ( ! file_exists( __DIR__ . '/detection/web-vitals.asset.php' ) ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

// phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error
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.

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

esc_html(
sprintf(
/* translators: 1: File path. 2: CLI command. */
'[Optimization Detective] ' . __( '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`'
)
),
E_USER_ERROR
);
}

define( 'OPTIMIZATION_DETECTIVE_VERSION', '0.1.0' );

require_once __DIR__ . '/helper.php';
Expand Down
Loading