Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use AJAX for activating features / plugins in Performance Lab #1646
Use AJAX for activating features / plugins in Performance Lab #1646
Changes from 14 commits
47f3938
9d97f59
6f172ad
337da6c
58c37d9
a59909d
f9faaea
770d462
e6710a3
c3d3588
fdf7e0b
b3ecc98
c713e47
1ebe323
c134fe2
369487f
d84e9bd
9bee27e
ce217ba
fa5d869
3a333ae
6250658
a8d89a8
c593ae6
e0691bf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 parameterslug
would be directly in the route path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 calledslug
orid
oridentifier
for example. The resource identifier being part of the endpoint path makes that easier to work with.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 hasslug
andsettingsUrl
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
andGET /plugins/<slug>
(the latter of which return an object that includes the settings URL) is best in line with the API design guidelines.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixarntz Additionally, there is also already a way to activate a plugin using the REST API by setting the
status
field on the entity toactive
, without resorting to a special endpoint naming scheme: https://developer.wordpress.org/rest-api/reference/plugins/#update-a-pluginHowever, 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 itfeature: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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toactive
, 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So let's go with
POST /features/<slug>:activate
andGET /features/<slug>
then?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where
GET /features/<slug>
returns an object with one key,settingsUrl
which may be either astring
ornull
, right? Sounds good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above, better would be:
This way the two routes are also consistently centered around a specific plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in this case it's just a regular REST API endpoint. No need for any workaround for a non-RESTy
POST
request, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you mean? How is this a workaround?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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 justGET
a noun. So so the normal REST design makes more sense to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already restricted to the PL plugins, is it not? It's using
perflab_sanitize_plugin_slug()
which returnsnull
if it is not a valid PL plugin slug. But I also suggested an alternative in https://github.com/WordPress/performance/pull/1646/files#r1840894846So I think this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make more sense to return a 404 status code here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The endpoint is for a settings URL entity though. You provide the slug of the plugin to get the settings URL. This suggestion is intended to be paired with https://github.com/WordPress/performance/pull/1646/files#r1840919242 so that if you
GET /plugin-settings-url/foo
and there is no settings URL for that plugin Foo, then it returns a 404.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we implement the
404
approach browser will log error in console for all the plugins which does not provide settings URL. As the browser logs error for404
status code returned request even if we wrap it in try catch. How should I approach this issue?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, maybe a 404 is too noisy. The endpoint could just return
null
in that case as opposed to the URL string.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
null
I have the concern that this may also be the result of a erroring JSON-decode operation.Maybe we return an object with just
pluginSettingsURL
key, which is either the URL orfalse
? That feels reasonable to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON decode where? If in JavaScript then this is
JSON.parse()
throws an error when a parse error occurs.I just tried patching the comments endpoint to return bad JSON intentionally:
Then if I do:
I get an JS error:
I don't feel strongly against wrapping the response in an object, however.