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

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.

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

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.

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.

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.

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.

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

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?

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.

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.

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

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.

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

See above, better would be:

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:

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.

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.

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.

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.

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.

See above, better would be:

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

Since this validates against the list of PL features/plugins, should we include an enum entry here with the possible values?

If that feels like too much, I think we should at least mention it in the description, e.g. "Must be one of the Performance Lab feature slugs."

I initially proposed using an enum here, but it won't work unless we then also always require the additional admin-only PHP files used in PerfLab. So what is implemented with this validate_callback is a sort of just-in-time enum. This is already explained in the phpdoc for perflab_validate_slug_endpoint_arg().

SG. In that case let's just mention it in the description.

Like so? https://github.com/WordPress/performance/pull/1646/files#r1844278835