-
Notifications
You must be signed in to change notification settings - Fork 14
Add client-only ability registration #69
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
base: add/client-library
Are you sure you want to change the base?
Conversation
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## add/client-library #69 +/- ##
=====================================================
Coverage 79.29% 79.29%
Complexity 96 96
=====================================================
Files 9 9
Lines 541 541
=====================================================
Hits 429 429
Misses 112 112
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* name: 'my-plugin/navigate', | ||
* label: 'Navigate to URL', | ||
* description: 'Navigates to a URL within WordPress admin', | ||
* location: 'client', |
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.
Maybe location
should be a part of meta
? I made it a top level though like the Feature API had. This is also only used in the client package. The server will only know about server side abilities.
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 a broader question on how to best group abilities or describe where they can be executed and who can do it. Similarily to whether the permission callback is needed as noted in #69 (comment).
In WP Feature API, was there some assumption that client features are sent to the server? I don't fully understand why we would have to explicitly annotate that as this could be done through the registration process. In fact, we might end up in the situation where we have three types of abilities:
- server-side only with
execute_callback
in PHP - client-side only with
executeCallback
in JavaScript - hybrid with
execute_callback
in PHP, and an alternative implementation on the client throughexecuteCallback
in JavaScript
The last one hybrid
might be a good fit for entities from @wordpress/core-data
. On the client, you could take advantage of optimistic updates, caching, etc, but on the server you can directly perform the same operations through WP functions.
packages/client/src/api.ts
Outdated
} | ||
if ( ! ability.callback || typeof ability.callback !== 'function' ) { | ||
throw new Error( | ||
'Client abilities must include a callback function' |
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.
We add a number of strings here for errors. These are for developers, not end users, but I'm guessing we might still want to wrap this in gettext calls.
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.
In PHP, even developer errors go through translation pipeline. In JavaScript things differ substantially. I'm not sure what the origin is, but looking at Gutenberg codebase, it should be fine to leave as is if it isn't user facing.
Do we need permission callbacks on the client as well?
Probably the best choice. Curious, how big does this make the bundle?
What do mean with "already part of the ecosystem"? It's a mere dev dependency in Gutenberg for use in some tests. It's not in any of the published packages. That's a big difference and not really an argument for picking this dependency. |
ce5aa40
to
082dfdf
Compare
It's also used in WordPress Playground and a few other utils too. I included it to say we have opted to use it in other places, which to me is worth considering over another package. I haven't looked at the bundle size (yet), but I personally think it is worth it instead of needing to hit a |
I was thinking about this as well and it's worth discussing. I think it would be nice to avoid needing to hit the REST API for running these, but maybe just providing a callback is enough? I'm open to ideas here! |
https://bundlephobia.com/package/[email protected] ![]() It's a very reasonable size for the functionality it provides. In the long run we could reuse the logic in Gutenberg to validate block attributes schema in the development mode.
It looks like on the server, we will enforce permission callbacks to increase the security as explained by @johnbillion in: It still might be valuable to have a way to define permission callbacks that checks whether a user can do something in the UI that's only for priviliged users so they have more streamlined experience. Otherwise we risk AI shows too often an message about failed attempe to do something it is not allowed to do. Similarily, @galatanovidiu in this comment #62 (comment) proposed a new property that does more high-level checks whether certain abilities should be filtered out if it's known upfront (without providing specific input) they aren't allowed to use them. |
// Route to appropriate execution method | ||
if (ability.location === 'client') { | ||
return executeClientAbility(ability, input); | ||
} | ||
|
||
return executeServerAbility(ability, input); |
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.
Taking further my thoughts from https://github.com/WordPress/abilities-api/pull/69/files#r2343288632, would it be enough to use ability.callback
instead of the location
? For server-only ability it should neven be defined, so it'll need to do the apiFetch
call regardless.
// Only allow unregistering client abilities | ||
if (state[action.name].location !== 'client') { | ||
// eslint-disable-next-line no-console | ||
console.warn( | ||
`Cannot unregister server-side ability: ${action.name}` | ||
); | ||
return state; | ||
} | ||
const newState = { ...state }; | ||
delete newState[action.name]; |
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.
Interesting case. What if the intention of the developer is to prevent exposure and execution of some server-side abilities and they opt to do it through JavaScript? I know they could do it also with PHP, but it's a choice.
I'm curious to hear the rationale behind this additional check. Is there something that could break? Would it re-register when someone asks for the same ability?
@@ -27,28 +27,28 @@ export function getAbilities() { | |||
{ per_page: PER_PAGE, page: 1 } | |||
); | |||
|
|||
if ( ! firstPage || firstPage.length === 0 ) { | |||
dispatch( receiveAbilities( [] ) ); | |||
if (!firstPage || firstPage.length === 0) { |
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.
It's probably more feedback for the parent branch (on this diff it is most likely due to Prettier formatting), but this can be simplified by passing page: -1
which will automatically fetch all pages through a special fetch all middleware:
* 'client' means it's executed locally in the browser. | ||
* Defaults to 'server'. | ||
*/ | ||
location?: 'server' | 'client'; |
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 more I see the code, I'm incrasingly confident it's redundant as it can be inferred from the existance of the optional callback
property.
Aside, technically speaking a client-side ability could still use callback
to call purely server-side ability. I'm mostly pointing out this distinction is mostly about where the callback's execution starts.
* @param param Optional parameter name for error messages. | ||
* @return True if valid, error message string if invalid. | ||
*/ | ||
export function validateValueFromSchema( |
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 nice! We might even find it a better home in a standalone WordPress package. I see extensive test coverage in #70 👍🏻
* ``` | ||
*/ | ||
export function registerAbility(ability: ClientAbility): void { | ||
if (!ability.name) { |
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.
We should consider additional check to ensure we don't override accidentally an existing ability. This would mirror how it works on the server, and how WordPress usually handles registries.
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 shaping up nicely. I left my feedback for consideration and to better understand some decisions made.
This PR follows up on #60 and introduces client-side only abilities. This allows for new integrations that deal with things like UI elements, navigation, and client-side data (such as the block editor). In addition to our AI efforts, it's also meant as a way that the command palette can execute client-side abilities in the future.
It adds two new methods to manage the client-only abilities
registerAbility
andunregisterAbility
.registerAbility
takes the same shape aswp_register_ability
in addition to a client side callback function. A callback can take the providedinput_schema
, do an action, and return the providedoutput_schema
.Here is a simple example:
unregisterAbility
is a convenience method for removing client-side only abilities.Client-side abilities are saved in the store alongside server-side ones. They are marked specifically with a location property which is used to either call the callback or call the REST endpoint.
The input and output schemas are validated using Ajv and the
ajv-formats
plugin.I have configured Ajv to support the intersection of rules that JSON Schema draft-04, WordPress (a subset of JSON Schema draft-04), and providers like OpenAI and Anthropic support.
Ajv is already in use by Gutenberg so it is already part of the ecosystem.
Testing
I have created a dummy plugin that registers a few different abilities as an example: client-abilities-demo.zip
Open the developer console and paste the following:
await wp.abilities.listAbilities()
You should see the client side abilities (and any other server side abilities you registered) listed.
Paste in the following:
await wp.abilities.executeAbility('demo/get-current-user')
See that you get a simple user response object back.
Paste in the following:
See that you are redirected to the users.php screen.
Paste in the following:
See that a UI notice briefly shows on the user page.
Now we can test registering and unregistering our own ability:
Now test the ability:
See that the various responses and different console types are executed.
To test the validation, you can change the return type and see what happens if you try to return a different type or something invalid against the schema.
Finally, you can unregister the client-side ability:
wp.abilities.unregisterAbility('demo/console-log');
. Trying to execute it again will throw an error.Next Steps (not addressed in this PR)
Tests & CI setup for the whole client package have been added in #70