Use AJAX for activating features / plugins in Performance Lab #1646

b1ink0 · 2024-11-11T17:22:47Z

Summary

Relevant technical choices

Frontend / UX
- Features / plugins in Performance Lab are now activated using AJAX improving user experience.
- No page refresh when activating features / plugins.
- Multiple plugin can be activated as each plugin activation sends different AJAX request.
- The click event is added to the anchor link as if JavaScript is enabled then the default behaviour is prevented so it functions as a button else It behaves as a link.
Backend
- Removed perflab_print_plugin_progress_indicator_script function as it was previously used for adding a progress indicator to the activate button on click. This functionality is now moved to the separate JavaScript file which handles the activation of plugin.
- Added perflab-plugin-activate-ajax (script handle) script which handles activation of features / plugins using AJAX.
- Updated perflab_render_plugin_card function to add data attribute data-plugin-slug in plugin activate link.
- Added new REST API endpoints
  - performance-lab/v1/features/(?P<slug>[a-z0-9_-]+):activate
  - performance-lab/v1/features/(?P<slug>[a-z0-9_-]+)
Unable to Implement fully
- Regarding the setting URL which is displayed in the admin notices and the plugin card.
- I have implemented the settings URL part but it only works for the Modern Image Formats plugin.
- Context: Currently only two plugins provide the settings URL Modern Image Formats and Speculative Loading.
- It does not work with the Speculative Loading because of how this plugin is loaded.
  - This plugin gets loaded ( loaded in this context is when are its helper files are loaded like settings.php ) on after_setup_theme hook.
  - But when the AJAX callback is triggered this hook has already been executed. As the plugin is activated later and which requries the after_setup_theme hook to fully load the plugin which includes the settings URL.
  - The function which is executed in the after_setup_theme calls a closure function which is stored in the global variable ( plsr_pending_plugin_info for the Speculative Loading plugin ) this closure function fully loads the plugin.
- I tried and tested this could be fixed by calling the closure function in AJAX callback but we will be hardcoding the global variable name and will have to update the callback function if other plugins also add settings URL.
Demo

demo_updated.mp4

…x handling, add ajax callback for activation of plugin

github-actions · 2024-11-11T17:23:05Z

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

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

westonruter · 2024-11-11T17:51:04Z

plugins/performance-lab/includes/admin/load.php

+	// Enqueue plugin activate AJAX script and localize script data.
+	wp_enqueue_script(
+		'perflab-plugin-activate-ajax',
+		PERFLAB_PLUGIN_DIR_URL . 'includes/admin/plugin-activate-ajax.js',


This PERFLAB_PLUGIN_DIR_URL is plugin_dir_url( PERFLAB_MAIN_FILE ), but I believe here it would be better to use plugins_url():

Suggested change

PERFLAB_PLUGIN_DIR_URL . 'includes/admin/plugin-activate-ajax.js',

plugins_url( 'includes/admin/plugin-activate-ajax.js', PERFLAB_MAIN_FILE ),

The reason is that this function allows for filtering as needed.

@westonruter I don't think we should randomly change it here though. If not using plugins_url() leads to a problem, let's discuss that in a separate issue and implement properly if needed.

I've filed #1647 for this.

plugins/performance-lab/includes/admin/load.php

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

westonruter · 2024-11-11T17:55:38Z

plugins/performance-lab/load.php

@@ -22,6 +22,7 @@
 define( 'PERFLAB_VERSION', '3.5.1' );
 define( 'PERFLAB_MAIN_FILE', __FILE__ );
 define( 'PERFLAB_PLUGIN_DIR_PATH', plugin_dir_path( PERFLAB_MAIN_FILE ) );
+define( 'PERFLAB_PLUGIN_DIR_URL', plugin_dir_url( PERFLAB_MAIN_FILE ) );


If my suggestion above is accepted, this can then be removed.

Suggested change

define( 'PERFLAB_PLUGIN_DIR_URL', plugin_dir_url( PERFLAB_MAIN_FILE ) );

See above, let's discuss separately, it's not really relevant for this enhancement.

At least let's avoid introducing a new constant in this PR.

That's fair, we can simply use plugin_dir_url( PERFLAB_MAIN_FILE ) in the wp_enqueue_script() call directly. I'm personally not a fan of having constants for all this either, particularly as the values are filterable, like you're saying.

I've filed #1647 for the filtering.

westonruter · 2024-11-11T18:02:35Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+	 */
+	$( document ).on(
+		'click',
+		'.perflab-install-active-plugin',


What if someone clicks this button while another request is currently in flight? Should it short-circuit so as to avoid duplicating requests?

I think as the request could have been half finished we should not short-circuit the request. I think it will be better to just disable it until the request is finished. But as the click event is on the anchor tag we will have to add pointer-events: none CSS to the anchor link using JavaScript to disable it.

The pointer-events style wouldn't address the issue of the button being activated by keyboard.

As the click event is on the anchor tag and not button we will have to add some other work around for disabling on keyboard one is setting its tabindex attribute to -1.

I tested this but if user is already focused then tabindex will not work. One other way is to add a attribute like aria-disabled and then check on click event if it has that attribute then return from the execution. Should I add tabindex and aria-disabled both?

You could add the disabled attribute, but that wouldn't be good because it would lose the tab focus for users navigating with the keyboard. Instead of this, WordPress has style rules for when you add the disabled class to a button so that it will look disabled. Adding aria-disabled here too would be good. But then for how to detect whether request is currently in progress, you could just short-circuit the click handler if the button has the updating-message class, or if it has the disabled class, or if it has the aria-disabled attribute.

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

westonruter · 2024-11-11T18:04:49Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+	}
+
+	// @ts-ignore
+} )( window.jQuery );


Does it make sense to make jQuery a dependency? Would plain JS not be feasible?

+1, it would be great to use vanilla JavaScript rather than relying on jQuery, to set a good example for performance.

westonruter · 2024-11-11T18:09:11Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+					showAdminNotice(
+						__( 'Feature activated.', 'performance-lab' ),
+						'success',
+						pluginSettingsURL
+					);


In the demo video you showed that this notice is causing layout shifts, resulting in you accidentally clicking the title of a plugin instead of the Activate button. I don't think an admin notice is even needed here, because the button transitions from Activate to Activating... to Active.

westonruter · 2024-11-11T18:20:29Z

It does not work with the Speculative Loading because of how this plugin is loaded.

This plugin gets loaded ( loaded in this context is when are its helper files are loaded like settings.php ) on after_setup_theme hook.

But when the AJAX callback is triggered this hook has already been executed. As the plugin is activated later and which requries the after_setup_theme hook to fully load the plugin which includes the settings URL.

The function which is executed in the after_setup_theme calls a closure function which is stored in the global variable ( plsr_pending_plugin_info for the Speculative Loading plugin ) this closure function fully loads the plugin.

So the issue is that when a plugin is activated, its source files are included, but they are included after the after_setup_theme action has fired, so then it is too late for the plugin to initialize in order to add the plugin_action_links_* filter. Interesting.

Given that plugin activation inherently happens partyway through another WP execution lifecycle after many of the actions potentially used by the activated plugin have already fired, I think it makes sense to introduce a secondary endpoint exclusively for obtaining the settings link. So you'd have one endpoint for activating the plugin and another endpoint for getting the settings link. This should be more reliable across how plugins bootstrap themselves.

felixarntz

Thanks for the PR @b1ink0!

Not much to add from myself to the feedback @westonruter already provided. It would be great to not have to use jQuery for this.

felixarntz · 2024-11-11T18:43:05Z

plugins/performance-lab/includes/admin/load.php

+	wp_enqueue_script(
+		'perflab-plugin-activate-ajax',
+		PERFLAB_PLUGIN_DIR_URL . 'includes/admin/plugin-activate-ajax.js',
+		array( 'jquery', 'wp-i18n', 'wp-a11y' ),


Related to @westonruter's previous feedback, it would be great not to use jQuery for the functionality, but rather vanilla JavaScript (e.g. fetch() function).

westonruter · 2024-11-11T19:20:22Z

Given that plugin activation inherently happens partyway through another WP execution lifecycle after many of the actions potentially used by the activated plugin have already fired, I think it makes sense to introduce a secondary endpoint exclusively for obtaining the settings link. So you'd have one endpoint for activating the plugin and another endpoint for getting the settings link. This should be more reliable across how plugins bootstrap themselves.

This said, it seems like we can avoid having to introduce a separate endpoint by tweaking how Speculative Loading bootstraps. Instead of:

performance/plugins/speculation-rules/load.php

Lines 31 to 65 in 5fbd82e

    
           static function ( string $global_var_name, string $version, Closure $load ): void { 
        
           	if ( ! isset( $GLOBALS[ $global_var_name ] ) ) { 
        
           		$bootstrap = static function () use ( $global_var_name ): void { 
        
           			if ( 
        
           				isset( $GLOBALS[ $global_var_name ]['load'], $GLOBALS[ $global_var_name ]['version'] ) 
        
           				&& 
        
           				$GLOBALS[ $global_var_name ]['load'] instanceof Closure 
        
           				&& 
        
           				is_string( $GLOBALS[ $global_var_name ]['version'] ) 
        
           			) { 
        
           				call_user_func( $GLOBALS[ $global_var_name ]['load'], $GLOBALS[ $global_var_name ]['version'] ); 
        
           				unset( $GLOBALS[ $global_var_name ] ); 
        
           			} 
        
           		}; 
        
           		// Wait until after the plugins have loaded and the theme has loaded. The after_setup_theme action is used 
        
           		// because it is the first action that fires once the theme is loaded. 
        
           		add_action( 'after_setup_theme', $bootstrap, PHP_INT_MIN ); 
        
           	} 
        
           	// Register this copy of the plugin. 
        
           	if ( 
        
           		// Register this copy if none has been registered yet. 
        
           		! isset( $GLOBALS[ $global_var_name ]['version'] ) 
        
           		|| 
        
           		// Or register this copy if the version greater than what is currently registered. 
        
           		version_compare( $version, $GLOBALS[ $global_var_name ]['version'], '>' ) 
        
           		|| 
        
           		// Otherwise, register this copy if it is actually the one installed in the directory for plugins. 
        
           		rtrim( WP_PLUGIN_DIR, '/' ) === dirname( __DIR__ ) 
        
           	) { 
        
           		$GLOBALS[ $global_var_name ]['version'] = $version; 
        
           		$GLOBALS[ $global_var_name ]['load']    = $load; 
        
           	} 
        
           }

We could do this:

	static function ( string $global_var_name, string $version, Closure $load ): void {
		$needs_bootstrap = ! isset( $GLOBALS[ $global_var_name ] );

		// Register this copy of the plugin.
		if (
			// Register this copy if none has been registered yet.
			! isset( $GLOBALS[ $global_var_name ]['version'] )
			||
			// Or register this copy if the version greater than what is currently registered.
			version_compare( $version, $GLOBALS[ $global_var_name ]['version'], '>' )
			||
			// Otherwise, register this copy if it is actually the one installed in the directory for plugins.
			rtrim( WP_PLUGIN_DIR, '/' ) === dirname( __DIR__ )
		) {
			$GLOBALS[ $global_var_name ]['version'] = $version;
			$GLOBALS[ $global_var_name ]['load']    = $load;
		}

		if ( $needs_bootstrap ) {
			$bootstrap = static function () use ( $global_var_name ): void {
				if (
					isset( $GLOBALS[ $global_var_name ]['load'], $GLOBALS[ $global_var_name ]['version'] )
					&&
					$GLOBALS[ $global_var_name ]['load'] instanceof Closure
					&&
					is_string( $GLOBALS[ $global_var_name ]['version'] )
				) {
					call_user_func( $GLOBALS[ $global_var_name ]['load'], $GLOBALS[ $global_var_name ]['version'] );
					unset( $GLOBALS[ $global_var_name ] );
				}
			};

			// Wait until after the plugins have loaded and the theme has loaded. The after_setup_theme action is used
			// because it is the first action that fires once the theme is loaded.
			if ( did_action( 'after_setup_theme' ) ) {
				$bootstrap();
			} else {
				add_action( 'after_setup_theme', $bootstrap, PHP_INT_MIN );
			}
		}
	}

Where if after_setup_theme already fired, we then invoke the bootstrap logic immediately. This would fix the issue with Speculative Loading, but this would essentially be a one-off fix that wouldn't necessarily work for other plugins. For example, Optimization Detective initializes at the init action, and at that point it triggers its own od_init action for other plugins to register their own dependent logic. Image Prioritizer, for example, waits to load any of its logic until the od_init action fires. If the Optimization Detective plugin was already installed and active, and then someone uses Ajax to activate the Image Prioritizer plugin, then the od_init action will never fire either, resulting in this exact same problem. So I think it is better to let the PHP process to activate the plugin fully terminate and then to start a new PHP process in a new endpoint to obtain the settings URL. This will avoid all issues with hook ordering across all plugins.

… path

… for Speculative Loading plugin

mukeshpanchal27 · 2024-11-12T16:22:36Z

plugins/performance-lab/includes/admin/load.php

 	// Enqueue the a11y script.
 	wp_enqueue_script( 'wp-a11y' );


Do we need these enqueue now? as it already added through new script dependancy.

We could remove it as it is not being used anywhere other than the AJAX script as it already added in its dependancy. Should I remove it then?

westonruter · 2024-11-12T21:37:15Z

plugins/speculation-rules/load.php

Actually, in #1646 (comment) I just posed this change as a possibility but then recommended against it since it would only account for Speculative Loading. It wouldn't account for other plugins that have delayed bootstrapping. So to address the problem of accessing the settings link after a plugin is activated, I think the best solution is to make another request specifically to an endpoint that exposes the settings link URLs.

Aside: I think the REST API would be better for this instead of admin-ajax.

westonruter · 2024-11-12T21:38:56Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+		try {
+			const response = await fetch(
+				// @ts-ignore
+				perflabPluginActivateAjaxData.ajaxUrl,


Why not use the REST API? You could just use wp.apiFetch then without having to export perflabPluginActivateAjaxData to the frontend.

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

westonruter · 2024-11-12T21:42:33Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+	 */
+	$( document ).on(
+		'click',
+		'.perflab-install-active-plugin',


You could add the disabled attribute, but that wouldn't be good because it would lose the tab focus for users navigating with the keyboard. Instead of this, WordPress has style rules for when you add the disabled class to a button so that it will look disabled. Adding aria-disabled here too would be good. But then for how to detect whether request is currently in progress, you could just short-circuit the click handler if the button has the updating-message class, or if it has the disabled class, or if it has the aria-disabled attribute.

westonruter · 2024-11-12T21:43:44Z

plugins/performance-lab/includes/admin/load.php

+	wp_enqueue_script(
+		'perflab-plugin-activate-ajax',
+		plugins_url( 'includes/admin/plugin-activate-ajax.js', PERFLAB_MAIN_FILE ),
+		array( 'wp-i18n', 'wp-a11y' ),


As noted in a comment below, this can use wp-api-fetch instead if the REST API is used.

Suggested change

array( 'wp-i18n', 'wp-a11y' ),

array( 'wp-i18n', 'wp-a11y', 'wp-api-fetch' ),

westonruter · 2024-11-12T21:47:21Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+				target.style.pointerEvents = '';
+				target.parentElement.style.cursor = '';


Suggested change

target.style.pointerEvents = '';

target.parentElement.style.cursor = '';

westonruter · 2024-11-12T21:48:05Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+			newButton.type = 'button';
+			newButton.className = 'button button-disabled';
+			newButton.disabled = true;
+			newButton.textContent = __( 'Active', 'performance-lab' );
+			target.parentElement.style.cursor = '';
+
+			target.parentNode.replaceChild( newButton, target );


Maybe just update the existing button instead of replace the button? By keeping the same element in place, users with a keyboard won't have the focus removed from the document.

The target is actually <a> tag ( because it should be able to work even if JavaScript is disabled ) and to match the mark up of loaded page which is disabled button because of that I am replacing the target <a> tag with button.

I think the disabled class works with a link as well. I think you can keep it as a link instead of replacing it with a button.

westonruter · 2024-11-12T21:48:23Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+				const listItem = document.createElement( 'li' );
+				const anchor = document.createElement( 'a' );
+
+				anchor.setAttribute( 'href', pluginSettingsURL );


Suggested change

anchor.setAttribute( 'href', pluginSettingsURL );

anchor.href = pluginSettingsURL;

westonruter · 2024-11-12T21:48:39Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+			target.style.pointerEvents = '';
+			target.parentElement.style.cursor = '';


Suggested change

target.style.pointerEvents = '';

target.parentElement.style.cursor = '';

westonruter · 2024-11-12T21:49:02Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+
+			const responseData = await response.json();
+
+			if ( ! responseData.success ) {


Maybe this should throw an error because the same logic here is located in the catch block.

…load, Don't replace anchor with button

…ngs URL, Refactor JavaScript code to use apiFetch

westonruter

Getting closer!

westonruter · 2024-11-13T17:02:03Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+	 *
+	 * @param {MouseEvent} event - The click event object that is triggered when the user clicks on the document.
+	 *
+	 * @return {Promise<void>} - The asynchronous function returns a promise that resolves to void.


Nit: the hyphen can be removed since it is only used with @param to make it more readable. It's not seen in the docs for @return.

Suggested change

* @return {Promise<void>} - The asynchronous function returns a promise that resolves to void.

* @return {Promise<void>} The asynchronous function returns a promise that resolves to void.

westonruter · 2024-11-13T17:06:24Z

plugins/performance-lab/includes/admin/rest-api.php

+/**
+ * Route for activating plugin.
+ *
+ * @var string
+ */
+const PERFLAB_ACTIVATE_PLUGIN_ROUTE = '/activate-plugin';


Since this endpoint is not a noun, it may make more sense as /plugin:activate. See for example in Optimization Detective:

performance/plugins/optimization-detective/storage/rest-api.php

Lines 20 to 30 in 2bec0e8

/**

* Route for storing a URL Metric.

*

* Note the `:store` art of the endpoint follows Google's guidance in AIP-136 for the use of the POST method in a way

* that does not strictly follow the standard usage. Namely, submitting a POST request to this endpoint will either

* create a new `od_url_metrics` post, or it will update an existing post if one already exists for the provided slug.

*

* @link https://google.aip.dev/136

* @var string

*/

const OD_URL_METRICS_ROUTE = '/url-metrics:store';

Suggested change

/**

* Route for activating plugin.

*

* @var string

*/

const PERFLAB_ACTIVATE_PLUGIN_ROUTE = '/activate-plugin';

/**

* Route for activating plugin.

*

* Note the `:activate` art of the endpoint follows Google's guidance in AIP-136 for the use of the POST method in a way

* that does not strictly follow the standard usage.

*

* @link https://google.aip.dev/136

* @var string

*/

const PERFLAB_ACTIVATE_PLUGIN_ROUTE = '/plugin:activate;

Indeed this is not a great resource URI for a REST URL, as it's not truly RESTful. REST endpoints should be centered around a resource, in this case a plugin.

But even better would be something like: /plugins/<slug>:activate. This would also be more intuitive in that the required parameter slug would be directly in the route path.

Suggested change

/**

* Route for activating plugin.

*

* @var string

*/

const PERFLAB_ACTIVATE_PLUGIN_ROUTE = '/activate-plugin';

/**

* Route for activating plugin.

*

* @var string

*/

const PERFLAB_ACTIVATE_PLUGIN_ROUTE = '/plugins/(?P<slug>[a-z0-9_-]+):activate';

@felixarntz I was just curious what should be the criteria for deciding whether to add the slug to the URL itself or include it in the body.

@b1ink0 It's one of the best practices for designing RESTful APIs, they should be centered around resources. See for example https://google.aip.dev/121 and https://google.aip.dev/122. (While these links are for Google's best practices, the best practices are not really specific to Google.)

By centering the API endpoints and their names around resources and their hierarchies, you future-proof the API to remain consistent and intuitive even when you add more and more features to it.

Here, the resource is a plugin. That's why using something like /plugins/<slug> as foundation for the routes is a good starting point. Such an endpoint could be used to get data for a specific plugin, or update data for a plugin for example.

Activating a plugin is not one of the CRUD operations though, so that's where a custom method is needed. See https://google.aip.dev/136 in that regard: That's how /plugins/<slug>:activate makes sense.

For example, if we later needed a method to deactivate a plugin, that would be straightforward via /plugins/<slug>:deactivate. If we used /activate-plugin, this would need to be /deactivate-plugin. That's still somewhat consistent but not centered around the resource and less intuitive. For example, nothing in the name tells you that you have to provide a slug. Additionally, even if you guessed that some identifier for the plugin would be needed, you wouldn't know whether it needs to be a parameter called slug or id or identifier for example. The resource identifier being part of the endpoint path makes that easier to work with.

@westonruter Following up on the other thread (since it probably makes sense to center that conversation about the endpoint naming): Per https://google.aip.dev/122, I think what would make most sense for the settings URL would be to simply be a value on the object returned by GET /plugins/<slug>.

A settings URL is not part of a collection, because a plugin does not have multiple settings URL. So GET /plugins/<slug>/settings-url would not be right. Because it's not an action (as you previously stated), GET /plugins/<slug>:settings-url wouldn't be right either.

The settings URL is one scalar piece of data for a plugin, so it makes most sense to be exposed on a plugin object. Obviously for this PR we don't care about creating a fully fledged GET /plugins/<slug> endpoint, but that's fine. We can simply for now return an object that e.g. only has slug and settingsUrl properties. The schema for such an object could be extended in the future in case we ever found a need for it, without backward compatibility breaks.

So I think going with POST /plugins/<slug>:activate and GET /plugins/<slug> (the latter of which return an object that includes the settings URL) is best in line with the API design guidelines.

@felixarntz Additionally, there is also already a way to activate a plugin using the REST API by setting the status field on the entity to active, without resorting to a special endpoint naming scheme: https://developer.wordpress.org/rest-api/reference/plugins/#update-a-plugin

However, reusing this endpoint is more complicated since we also automatically install the plugin prior to activation, as well as installing and activating any dependencies.

So maybe we should keep our own endpoint for installation and activation but instead of calling it plugin:activate we call it feature:activate to differentiate it from general plugin activation, as these plugins represent performance features.

In the end, this API is almost guaranteed to be used 100% internally and so we don't really need to stress about it being getting it right forever.

I don't think the existing plugin endpoints in Core are a problem. We use a different namespace specific to Performance Lab, so I wouldn't worry about that. It's IMO not unreasonable to provide custom endpoints for similar use-cases under a different namespace when the Core endpoints do not satisfy the requirements we have.

Regarding updating a status field to active, that's how they chose to build it, but doesn't seem that great to me per https://google.aip.dev/216.

That said, I'd be okay using the terminology "features" instead of "plugins" for our endpoints, since that's also what we use in the UI, and I agree it can help differentiate.

So let's go with POST /features/<slug>:activate and GET /features/<slug> then?

Where GET /features/<slug> returns an object with one key, settingsUrl which may be either a string or null, right? Sounds good.

Exactly.

If we like, we could also include e.g. slug in the response object as that's simple enough and clarifies the intent of the endpoint even more (that it's supposed to return plugin information, so not necessarily just the settings URL).

westonruter · 2024-11-13T17:11:00Z

plugins/performance-lab/includes/admin/rest-api.php

+
+	$plugin_slug = perflab_sanitize_plugin_slug( wp_unslash( $params['slug'] ) );
+	if ( null === $plugin_slug ) {
+		return new WP_Error(
+			'invalid_plugin',
+			__( 'Invalid plugin slug provided.', 'performance-lab' ),
+			array( 'status' => 400 )
+		);
+	}


This can be eliminated in favor of adding the validation to the REST API arg.

Suggested change

$plugin_slug = perflab_sanitize_plugin_slug( wp_unslash( $params['slug'] ) );

if ( null === $plugin_slug ) {

return new WP_Error(

'invalid_plugin',

__( 'Invalid plugin slug provided.', 'performance-lab' ),

array( 'status' => 400 )

);

}

westonruter · 2024-11-13T17:12:08Z

plugins/performance-lab/includes/admin/rest-api.php

+		);
+	}
+
+	$plugin_settings_url = perflab_get_plugin_settings_url( $plugin_slug );


Suggested change

$plugin_settings_url = perflab_get_plugin_settings_url( $plugin_slug );

$plugin_settings_url = perflab_get_plugin_settings_url( $request['slug'] );

westonruter · 2024-11-13T17:12:49Z

plugins/performance-lab/includes/admin/rest-api.php

+	$params = $request->get_json_params();
+
+	// Ensure the 'slug' parameter is present.
+	if ( ! isset( $params['slug'] ) ) {
+		return new WP_Error(
+			'missing_parameter',
+			__( 'Missing required parameter "slug".', 'performance-lab' ),
+			array( 'status' => 400 )
+		);
+	}
+
+	$plugin_slug = perflab_sanitize_plugin_slug( wp_unslash( $params['slug'] ) );
+	if ( null === $plugin_slug ) {
+		return new WP_Error(
+			'invalid_plugin',
+			__( 'Invalid plugin slug provided.', 'performance-lab' ),
+			array( 'status' => 400 )
+		);
+	}


This code isn't needed because the validation is handed by the REST API, per above.

Suggested change

$params = $request->get_json_params();

// Ensure the 'slug' parameter is present.

if ( ! isset( $params['slug'] ) ) {

return new WP_Error(

'missing_parameter',

__( 'Missing required parameter "slug".', 'performance-lab' ),

array( 'status' => 400 )

);

}

$plugin_slug = perflab_sanitize_plugin_slug( wp_unslash( $params['slug'] ) );

if ( null === $plugin_slug ) {

return new WP_Error(

'invalid_plugin',

__( 'Invalid plugin slug provided.', 'performance-lab' ),

array( 'status' => 400 )

);

}

westonruter · 2024-11-13T17:37:47Z

plugins/performance-lab/includes/admin/rest-api.php

+
+	$params = $request->get_json_params();
+
+	// Ensure the 'slug' parameter is present.
+	if ( ! isset( $params['slug'] ) ) {
+		return new WP_Error(
+			'missing_parameter',
+			__( 'Missing required parameter "slug".', 'performance-lab' ),
+			array( 'status' => 400 )
+		);
+	}


This is not needed because the REST API schema enforces that the slug parameter will be present.

Suggested change

$params = $request->get_json_params();

// Ensure the 'slug' parameter is present.

if ( ! isset( $params['slug'] ) ) {

return new WP_Error(

'missing_parameter',

__( 'Missing required parameter "slug".', 'performance-lab' ),

array( 'status' => 400 )

);

}

westonruter · 2024-11-13T17:41:17Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+			// Fetch the plugin settings URL via the REST API.
+			const settingsResponse = await apiFetch( {
+				path: '/performance-lab/v1/plugin-settings-url',
+				method: 'POST',
+				data: { slug: pluginSlug },
+			} );
+
+			a11y.speak( __( 'Plugin activated.', 'performance-lab' ) );
+
+			target.textContent = __( 'Active', 'performance-lab' );
+			target.classList.remove( 'updating-message' );
+			target.classList.add( 'disabled' );
+
+			const actionButtonList = document.querySelector(
+				`.plugin-card-${ pluginSlug } .plugin-action-buttons`
+			);
+
+			if ( settingsResponse?.pluginSettingsURL && actionButtonList ) {
+				const listItem = document.createElement( 'li' );
+				const anchor = document.createElement( 'a' );
+
+				anchor.href = settingsResponse?.pluginSettingsURL;
+				anchor.textContent = __( 'Settings', 'performance-lab' );
+
+				listItem.appendChild( anchor );
+				actionButtonList.appendChild( listItem );
+			}


All of the logic specifically for getting the settings link could be moved to a separate try/catch block. If the settings URL endpoint returns a 404, then an error will be thrown and then there is no need to check if settingsResponse?.pluginSettingsURL exists.

Suggested change

// Fetch the plugin settings URL via the REST API.

const settingsResponse = await apiFetch( {

path: '/performance-lab/v1/plugin-settings-url',

method: 'POST',

data: { slug: pluginSlug },

} );

a11y.speak( __( 'Plugin activated.', 'performance-lab' ) );

target.textContent = __( 'Active', 'performance-lab' );

target.classList.remove( 'updating-message' );

target.classList.add( 'disabled' );

const actionButtonList = document.querySelector(

`.plugin-card-${ pluginSlug } .plugin-action-buttons`

);

if ( settingsResponse?.pluginSettingsURL && actionButtonList ) {

const listItem = document.createElement( 'li' );

const anchor = document.createElement( 'a' );

anchor.href = settingsResponse?.pluginSettingsURL;

anchor.textContent = __( 'Settings', 'performance-lab' );

listItem.appendChild( anchor );

actionButtonList.appendChild( listItem );

}

a11y.speak( __( 'Plugin activated.', 'performance-lab' ) );

target.textContent = __( 'Active', 'performance-lab' );

target.classList.remove( 'updating-message' );

target.classList.add( 'disabled' );

westonruter · 2024-11-13T17:42:00Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+		try {
+			// Activate the plugin via the REST API.
+			await apiFetch( {
+				path: '/performance-lab/v1/activate-plugin',


Suggested change

path: '/performance-lab/v1/activate-plugin',

path: '/performance-lab/v1/plugin:activate',

westonruter · 2024-11-13T17:48:22Z

plugins/performance-lab/includes/admin/rest-api.php

+ *
+ * @var string
+ */
+const PERFLAB_PLUGIN_SETTINGS_URL_ROUTE = '/plugin-settings-url';


Suggested change

const PERFLAB_PLUGIN_SETTINGS_URL_ROUTE = '/plugin-settings-url';

const PERFLAB_PLUGIN_SETTINGS_URL_ROUTE = '/plugin-settings-url/(?P<slug>[a-z0-9_-]+)';

See above, better would be:

Suggested change

const PERFLAB_PLUGIN_SETTINGS_URL_ROUTE = '/plugin-settings-url';

const PERFLAB_PLUGIN_SETTINGS_URL_ROUTE = '/plugins/(?P<slug>[a-z0-9_-]+):settings-url';

This way the two routes are also consistently centered around a specific plugin.

But in this case it's just a regular REST API endpoint. No need for any workaround for a non-RESTy POST request, right?

How do you mean? How is this a workaround?

I mean, as I understand, a workaround like this is needed when the REST API endpoint is not a noun and/or the existing HTTP methods don't make semantic sense. According to AIP-136 on custom methods:

Resource-oriented design (AIP-121) uses custom methods to provide a means to express arbitrary actions that are difficult to model using only the standard methods.

What we discussed above in https://github.com/WordPress/performance/pull/1646/files#r1841160336 is to add a verb to the endpoint for use with POST. But here for the settings URL, it's a classic REST API endpoint, like most other REST API endpoints in WordPress, where we're just GET a noun. So so the normal REST design makes more sense to me.

That's a good point. Though the current naming still seems incorrect in that it's the settings URL of a specific plugin. So a more appropriate name would be:

Suggested change

const PERFLAB_PLUGIN_SETTINGS_URL_ROUTE = '/plugin-settings-url';

const PERFLAB_PLUGIN_SETTINGS_URL_ROUTE = '/plugins/(?P<slug>[a-z0-9_-]+)/settings-url';

This way it is a resource, but under the plugin it belongs to. Arguably with that approach, the 404 error if there's no settings URL makes more sense too.

See #1646 (comment), discard my previous comment.

Let's continue that discussion on the other thread, to center the conversation about endpoint naming in one place.

westonruter · 2024-11-13T17:50:16Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+			target.textContent = __( 'Activate', 'performance-lab' );
+		}
+	}
+


Per above:

Suggested change

try {

// Fetch the plugin settings URL via the REST API.

const settingsResponse = await apiFetch( {

path: `/performance-lab/v1/plugin-settings-url/${ pluginSlug }`,

method: 'GET',

} );

const actionButtonList = document.querySelector(

`.plugin-card-${ pluginSlug } .plugin-action-buttons`

);

const listItem = document.createElement( 'li' );

const anchor = document.createElement( 'a' );

anchor.href = settingsResponse.pluginSettingsURL;

anchor.textContent = __( 'Settings', 'performance-lab' );

listItem.appendChild( anchor );

actionButtonList.appendChild( listItem );

} catch ( error ) {}

Hi, There will be UX issue if settings URL try catch is moved after the changing DOM for the anchor tag / button. As it will become disabled after first request is finished signaling the user that the action is done but then the settings URL fetch request after some time if the plugin has settings URL then the DOM will be modified after some delay.

Also I think we should only send the settings URL request when the activate request is successful.

Should I just move the settings URL try catch inside the activate plugin try catch and handle DOM of anchor tag / button after the settings URL try catch block?

delayed_activation.mp4

Yeah, that works.

Also, if the endpoint returns null instead of a 404, then no separate try/catch would be needed.

felixarntz

@b1ink0 This looks like it's on the right track.

I think going with a REST API is great, though it would have been okay to go with Admin AJAX for this kind of endpoint as well, that would have been a bit simpler. If we use the REST API, I think we should ensure we use it correctly. Most of my feedback is centered around that.

But this is coming along nicely!

felixarntz · 2024-11-13T20:54:21Z

plugins/performance-lab/includes/admin/plugin-activate-ajax.js

+	}
+
+	// Attach the event listener.
+	document.addEventListener( 'click', handlePluginActivationClick );


Why does this add a listener for the entire document instead of adding listeners to the buttons specifically?

I feel like that would be a more reasonable choice, as it avoids having the listener fire for more or less every click on the page. Since the buttons are present in the HTML response from the beginning, I don't see a need for listening on document.

felixarntz · 2024-11-13T20:59:19Z

plugins/performance-lab/includes/admin/rest-api.php

+		return new WP_REST_Response(
+			array(
+				'success'           => true,
+				'pluginSettingsURL' => false,
+			)
+		);


I disagree here. The caller provides the plugin slug, not an identifier for the settings URL. I'd argue a 404 would only be appropriate if the plugin slug was for a plugin that's not active or doesn't exist. But if the plugin is active and just doesn't provide a settings URL, then it shouldn't be a 404, since the endpoint's sole purpose is to retrieve the settings URL for an active plugin.

felixarntz · 2024-11-13T21:02:40Z

plugins/performance-lab/includes/admin/rest-api.php

+ * @param WP_REST_Request $request Request.
+ * @return WP_REST_Response|WP_Error Response.
+ */
+function perflab_handle_activate_plugin( WP_REST_Request $request ) {


Now that we expose this as a REST endpoint, I wonder whether we should restrict it to plugins that are part of the Performance Lab program. Otherwise we may need to deal with other implications related to arbitrary other plugins. I feel like limiting to our own would be a good idea, for both of these endpoints.

For example, we may make certain assumptions about our plugins, that aren't reasonable to make for any plugin.

This is already restricted to the PL plugins, is it not? It's using perflab_sanitize_plugin_slug() which returns null if it is not a valid PL plugin slug. But I also suggested an alternative in https://github.com/WordPress/performance/pull/1646/files#r1840894846

So I think this is fine.

felixarntz · 2024-11-13T21:06:14Z

plugins/performance-lab/includes/admin/rest-api.php

+/**
+ * Route for activating plugin.
+ *
+ * @var string
+ */
+const PERFLAB_ACTIVATE_PLUGIN_ROUTE = '/activate-plugin';


Indeed this is not a great resource URI for a REST URL, as it's not truly RESTful. REST endpoints should be centered around a resource, in this case a plugin.

But even better would be something like: /plugins/<slug>:activate. This would also be more intuitive in that the required parameter slug would be directly in the route path.

Suggested change

/**

* Route for activating plugin.

*

* @var string

*/

const PERFLAB_ACTIVATE_PLUGIN_ROUTE = '/activate-plugin';

/**

* Route for activating plugin.

*

* @var string

*/

const PERFLAB_ACTIVATE_PLUGIN_ROUTE = '/plugins/(?P<slug>[a-z0-9_-]+):activate';

felixarntz · 2024-11-13T21:06:57Z

plugins/performance-lab/includes/admin/rest-api.php

+ *
+ * @var string
+ */
+const PERFLAB_PLUGIN_SETTINGS_URL_ROUTE = '/plugin-settings-url';


See above, better would be:

Suggested change

const PERFLAB_PLUGIN_SETTINGS_URL_ROUTE = '/plugin-settings-url';

const PERFLAB_PLUGIN_SETTINGS_URL_ROUTE = '/plugins/(?P<slug>[a-z0-9_-]+):settings-url';

This way the two routes are also consistently centered around a specific plugin.

westonruter · 2024-11-13T21:19:28Z

plugins/performance-lab/includes/admin/rest-api.php

+	return new WP_REST_Response(
+		array(
+			'success'           => true,
+			'pluginSettingsURL' => esc_url_raw( $plugin_settings_url ),
+		)
+	);


Alternatively, this can just return the JSON string:

Suggested change

return new WP_REST_Response(

array(

'success' => true,

'pluginSettingsURL' => esc_url_raw( $plugin_settings_url ),

)

);

return new WP_REST_Response( $plugin_settings_url );

…efactor to separate plugin activate and settings URL request in there own try catch

…I routes

b1ink0 added 6 commits November 7, 2024 18:21

Update print plugin progress indicator script function to support aja…

47f3938

…x handling, add ajax callback for activation of plugin

Add admin notice dynamically, Update to use async/await

9d97f59

Add settings URL of plugin in admin notice

6f172ad

Move inline JavaScript to separate file

337da6c

Refactor JavaScript code to use jQuery

58c37d9

Update to use IIFE, Remove unnecessary doc comment

a59909d

b1ink0 requested review from mukeshpanchal27 and felixarntz as code owners November 11, 2024 17:22

Merge branch 'trunk' into update/use-ajax-for-activate-plugin

f9faaea

westonruter added this to the performance-lab n.e.x.t milestone Nov 11, 2024

westonruter requested changes Nov 11, 2024

View reviewed changes

westonruter reviewed Nov 11, 2024

View reviewed changes

felixarntz reviewed Nov 11, 2024

View reviewed changes

westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only labels Nov 11, 2024

b1ink0 and others added 4 commits November 12, 2024 12:38

Merge branch 'WordPress:trunk' into update/use-ajax-for-activate-plugin

770d462

Remove admin notice part, Remove constant, Use plugin_url for JS file…

e6710a3

… path

Refactor to use vanilla JavaScript

c3d3588

Fix duplicate request on multiple click, Fix settings URL not showing…

fdf7e0b

… for Speculative Loading plugin

b1ink0 requested review from westonruter and felixarntz November 12, 2024 11:19

mukeshpanchal27 reviewed Nov 12, 2024

View reviewed changes

westonruter requested changes Nov 12, 2024

View reviewed changes

b1ink0 and others added 3 commits November 13, 2024 14:00

Remove wp-a11y enqueue, Revert changes to Speculative Loading plugin …

b3ecc98

…load, Don't replace anchor with button

Add REST API endpoints for activating plugin and getting plugin setti…

c713e47

…ngs URL, Refactor JavaScript code to use apiFetch

Merge branch 'trunk' into update/use-ajax-for-activate-plugin

1ebe323

b1ink0 requested a review from westonruter November 13, 2024 13:18

b1ink0 requested a review from mukeshpanchal27 November 13, 2024 13:18

westonruter requested changes Nov 13, 2024

View reviewed changes

felixarntz requested changes Nov 13, 2024

View reviewed changes

westonruter reviewed Nov 13, 2024

View reviewed changes

b1ink0 and others added 4 commits November 14, 2024 14:32

Update REST API routes, Add validation to args of REST API request, R…

c134fe2

…efactor to separate plugin activate and settings URL request in there own try catch

Update REST API routes, Update JavaScript code to use updated REST AP…

369487f

…I routes

Merge branch 'trunk' into update/use-ajax-for-activate-plugin

d84e9bd

Fix inline comments and error messages

9bee27e

b1ink0 requested review from felixarntz and westonruter November 15, 2024 06:20

	PERFLAB_PLUGIN_DIR_URL . 'includes/admin/plugin-activate-ajax.js',
	plugins_url( 'includes/admin/plugin-activate-ajax.js', PERFLAB_MAIN_FILE ),

	array( 'wp-i18n', 'wp-a11y' ),
	array( 'wp-i18n', 'wp-a11y', 'wp-api-fetch' ),

		target.style.pointerEvents = '';
		target.parentElement.style.cursor = '';

	anchor.setAttribute( 'href', pluginSettingsURL );
	anchor.href = pluginSettingsURL;


		const responseData = await response.json();

		if ( ! responseData.success ) {

	* @return {Promise<void>} - The asynchronous function returns a promise that resolves to void.
	* @return {Promise<void>} The asynchronous function returns a promise that resolves to void.

	/**
	* Route for storing a URL Metric.
	*
	* Note the `:store` art of the endpoint follows Google's guidance in AIP-136 for the use of the POST method in a way
	* that does not strictly follow the standard usage. Namely, submitting a POST request to this endpoint will either
	* create a new `od_url_metrics` post, or it will update an existing post if one already exists for the provided slug.
	*
	* @link https://google.aip.dev/136
	* @var string
	*/
	const OD_URL_METRICS_ROUTE = '/url-metrics:store';

	$plugin_settings_url = perflab_get_plugin_settings_url( $plugin_slug );
	$plugin_settings_url = perflab_get_plugin_settings_url( $request['slug'] );

	path: '/performance-lab/v1/activate-plugin',
	path: '/performance-lab/v1/plugin:activate',

	const PERFLAB_PLUGIN_SETTINGS_URL_ROUTE = '/plugin-settings-url';
	const PERFLAB_PLUGIN_SETTINGS_URL_ROUTE = '/plugin-settings-url/(?P<slug>[a-z0-9_-]+)';

Use AJAX for activating features / plugins in Performance Lab #1646

Are you sure you want to change the base?

Use AJAX for activating features / plugins in Performance Lab #1646

Conversation

b1ink0 commented Nov 11, 2024 • edited Loading

Summary

Relevant technical choices

github-actions bot commented Nov 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter Nov 11, 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

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 Nov 11, 2024

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Nov 11, 2024

Choose a reason for hiding this comment

b1ink0 Nov 12, 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

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

felixarntz Nov 14, 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

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 Nov 14, 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

b1ink0 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

b1ink0 commented Nov 11, 2024 •

edited

Loading

github-actions bot commented Nov 11, 2024 •

edited

Loading

westonruter Nov 11, 2024 •

edited

Loading

b1ink0 Nov 12, 2024 •

edited

Loading

felixarntz Nov 14, 2024 •

edited

Loading

westonruter Nov 14, 2024 •

edited

Loading

b1ink0 Nov 14, 2024 •

edited

Loading

felixarntz Nov 13, 2024 •

edited

Loading