-
Notifications
You must be signed in to change notification settings - Fork 20
Add client library tests and code quality infrastructure #70
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
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-only-abilities #70 +/- ##
===============================================================
+ Coverage 77.89% 81.78% +3.88%
Complexity 103 103
===============================================================
Files 9 15 +6
Lines 552 741 +189
Branches 0 75 +75
===============================================================
+ Hits 430 606 +176
- Misses 122 135 +13
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:
|
03b9f49
to
bf094ef
Compare
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 reviewed this PR and left my feedback to consider.
getAbility, | ||
registerAbility, | ||
unregisterAbility, | ||
executeAbility, |
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.
executeAbility
would be the most considerable difference with the server. This is more high-level feedback, which doesn't have to be immediately addressed, more like a discussion starter.
Would it make sense to mirror that on the server and offer wp_execute_ability
?
In the same fashion, could getAbility
always return an object that would allow calling:
const ability = await getAbility( 'my/ability' );
const result = await ability.execute( input );
Aside, all tests deal with sync callbacks, but I noticed that some examples documented could be async. We also have server abilities that don't have the callback, but maybe we could provide a general handler there, 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.
wp_execute_ability
does seem like it could be useful. I need to think about this discussion more but wanted to note that I saw it.
ability.execute
feels a little odd to me at initial glance 🤔
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.
@copilot, what are your thoughts on this thread?
Let's see if it can respond 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.
ability.execute feels a little odd to me at initial glance 🤔
Right, it doesn't translate that well into JavaScript code.
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 made the mistake of reviewing #60 before looking at this. Tl;Dr
-
I think we'll all have a much easier time if we treat this like a monorepo, use NPM workspaces and run all the dev/build commands top-level, and more generally follow the patterns used in the Gutenberg repo (whether that's the eventual home of a merged package or not, it's still peer-reviewed prior art that contributors are already familiar with)
-
Either way, not sure I follow the need for two GH workflows. If this tiny public repo really needs some
paths
filtering, then we can do it with https://github.com/dorny/paths-filter or similar. (Otherwise at least filterpackages/*
out oftest.yml
and do all the JS related stuff, including testing injs-lint.ymljs-test.yml
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.
Commenting only on the documentation updates (which is where my focus has been for this project), it all looks good to me.
398612b
to
36da8ae
Compare
9375c13
to
5149542
Compare
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 have more feedback for this PR other than a nitpick regarding usage console.warning
over warnining
from @wordpress/warning
. It looks like all my important concerns raised where addressed.
It still needs some testing, which I plan to do tomorrow.
@emdashcodes, I'm not entirely sure how you would like to go about merging the code, but since it isn't targeting trunk
I figured out, I approve it in case you would like to combine #69 and #70 together. It still will require testing, so this way I would review changes applied to #69 and test both PR at once.
* Client only abilities * fix: Add proper server ability validation in registerAbility * feat: Add ClientAbility and ServerAbility types for better type safety * refactor: Remove location field, use callback presence for execution type * feat: add permissionCallback on the client * update: check if ability is already registered in registerAbility * fix: wrap client errors in i18n * Add client library tests and code quality infrastructure (#70) * Add testing & linting infa * Remove uncessary combined checks * Add client tests * test: Use proper TypeScript types instead of 'as any' in tests * Fix typescript check for extra server check * test: update tests for client-only abilities API changes * chore: add build:client and dev:client scripts to root package.json * fix: formatting * fix: prettier issues * fix: apply Prettier formatting to README and validation.ts * deps: remove @types/wordpress__api-fetch since types are already shipped * fix: move validation to the store * fix: validate ability name uses same format as server * tests: clean up CI checks * deps: fix dev dependencies and remove extra package-lock.json * fix: rename main dev and build commands * fix: better match server validation rules for schema * Fix remaining validation issues Co-authored-by: emdashcodes <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: justlevine <[email protected]> Co-authored-by: jonathanbossenger <[email protected]> --------- Co-authored-by: emdashcodes <[email protected]> Co-authored-by: gziolo <[email protected]> Co-authored-by: justlevine <[email protected]> Co-authored-by: jonathanbossenger <[email protected]> Co-authored-by: swissspidy <[email protected]>
This PR adds test coverage and code quality to the client package. It follows-up on the following two PRs: #60 and #69.
It adds:
wp-scripts
wp-scripts
Things are broken down into a few separate commits to make things easier to review. Apologizes again for
package-lock.json
inflation.Testing
See that all CI checks pass below
Locally