-
Notifications
You must be signed in to change notification settings - Fork 50
Add category listing and validation to client package #121
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #121 +/- ##
============================================
+ Coverage 88.50% 89.17% +0.66%
Complexity 162 162
============================================
Files 19 19
Lines 1140 1238 +98
Branches 89 117 +28
============================================
+ Hits 1009 1104 +95
- Misses 131 134 +3
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:
|
|
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. |
7185ec4 to
3d92902
Compare
JasonTheAdams
left a comment
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 looking pretty good! One question comes to mind: Should it be possible to register categories here the same way abilities can? If a user is registering abilities on the front-end, is it also possible they may be introducing new categories of abilities? I would think so, actually, in such cases as interacting with the block editor.
A category here obviously shouldn't conflict with a category already registered from the server. But I think that's about it.
3d92902 to
1e273c3
Compare
1e273c3 to
0366450
Compare
Yeah, I think that we should allow for this, and that there's going to be a lot of use cases where a set of abilities is just all client-side. I went ahead and added support for this here: cc55e76. I have also updated the docs and the PR description to include the new changes. |
gziolo
left a comment
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 looks close to ready. I did first pass of review of production code and it looks solid 👍
|
@gziolo Thanks for the review! I've pushed a couple final changes here. |
|
I’ll have another look later. The follow up changes look good after quick check 👍 |
| await resolveSelect( STORE_NAME ).getAbilityCategories(); | ||
| const existingCategory = select.getAbilityCategory( slug ); | ||
| if ( existingCategory ) { | ||
| throw new Error( | ||
| sprintf( 'Category "%s" is already registered.', slug ) | ||
| ); | ||
| } |
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.
@emdashcodes Just double-checking: It's this await resolveSelect here that ensures the server categories are loaded prior to checking for a collision, 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.
Yup! See https://developer.wordpress.org/block-editor/reference-guides/packages/packages-data/#resolveselect. The promise here will resolve once the resolvers fetch from the entity store, which will fetch from the REST API if needed.
| } | ||
| ``` | ||
|
|
||
| #### `registerAbility( ability: Ability ): Promise<void>` |
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 will need an update in another document the example with missing await, too:
A larger question for later is how to avoid redundancy between these two documents. Is this one auto-generated from JSDoc?
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.
Thanks, I updated this document with all the new methods and updated the await in the example.
A larger question for later is how to avoid redundancy between these two documents. Is this one auto-generated from JSDoc?
This one is not auto-generated. I'm not sure the best way to keep them in sync to be honest.
I do think it's valuable for the client package README to have a function definition and such. Especially if it gets split off into Gutenberg. But we also don't want to duplicate stuff in multiple places obviously..
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.
Gutenberg has tooling in place that allows auto-generating the list of public methods based on the JSDoc, so that would cover the README file nicely, as these methods are very well documented.
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 a bit complicated to handle entities for abilities, and ability categories, while it's possible to register more items on the client for both registries. We probably need a small tweak as outlined in https://github.com/WordPress/abilities-api/pull/121/files#r2437279266, but it also raises a larger question what would be the ideal design to take advantage of the fact that these entities are already nicely abstracted, and maybe, we could avoid using these resolvers, and instead make selectors more complex with direct calls that resolvers do today. I have no idea if that is feasible, but I am leaving a note to revisit later.
…ng from server if a client category is registered
gziolo
left a comment
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.
No more feedback from my side. I'm glad to see we reached feature parity with the server implementaiton for categories 🎉
* add: category listing endpoints for the REST API * add: client support for category listing and valdating before registering an ability * fix: increase test coverage * add: registerAbilityCategory and unregisterAbilityCategory * fix: use resolveSelect and use createSelector for getAbilityCategories * fix: update docs/ on client, clarify selector call, and fix for loading from server if a client category is registered Co-authored-by: emdashcodes <emdashcodes@git.wordpress.org> Co-authored-by: JasonTheAdams <jason_the_adams@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org>
This PR adds improved category support to the client package. It adds methods for listing categories (
getAbilityCategories,getAbilityCategory), methods for registering and unregistering categories (registerAbilityCategory,unregisterAbilityCategory), and it validates ability categories during ability registration.Follows up on #102 (categories system), #114 (basic category support), and #120 (REST endpoints).
Why
In the previous client PR we only addressed filtering abilities by category slug but had no way to discover available categories or validate that a category exists before registering an ability. There was also no way to register a category only on the client side.
How
getAbilityCategories()function to fetch all categoriesgetAbilityCategory(slug)function to fetch a single categoryregisterAbilityCategoryto register a category on the client sideunregisterAbilityCategoryto remove a category on the client sideBreaking Change
registerAbility()now returnsPromise<void>instead ofvoid. This is. to. ensure that categories are loaded on firstregisterAbility()call if store is empty. Since categories are already a breaking change, I feel like this is OK. It does still work without awaiting, but it's possible the categories are not loaded before the register / validation check.Testing Steps
Run
npm run test:clientRegister a few test categories and abilities
Open WordPress admin in browser with dev tools console
Test
getAbilityCategories():Test
getAbilityCategory():Test
registerAbility()validation:Test with a valid category:
Test with an invalid category: