Core Data: Support entity queries in the 'useResourcePermissions' hook #63653
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
added
[Type] Enhancement
|
Size Change: +19 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
Mamaduka
force-pushed
the
update/use-resource-permissions-hook
branch
from
925efaf
to
ec2abb7
Compare
Mamaduka
commented
Jul 17, 2024
Mamaduka
commented
Jul 17, 2024
Comment on lines
+156
to
+159
| const permissions = useResourcePermissions( { | ||
| kind: 'postType', | ||
| name: postType, | ||
| } ); |
There was a problem hiding this comment.
Removes unnecessary checks for both entities, which later might have been disregarded. Reduces the possible OPTIONS request to one.
There was a problem hiding this comment.
Nice one 👍 I appreciate the resulting cleanup!
Mamaduka
commented
Jul 17, 2024
packages/edit-widgets/src/components/widget-areas-block-editor-provider/index.js
Show resolved
Hide resolved
Mamaduka
commented
Jul 17, 2024
Mamaduka
commented
Jul 17, 2024
Mamaduka
requested review from
draganescu,
adamziel,
kevin940726,
ajitbohra and
nerrad
as code owners
|
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. |
|
This is ready for the review. cc @WordPress/gutenberg-core |
tyxla
reviewed
Jul 18, 2024
| return useQuerySelect( | ||
| ( resolve ) => { | ||
| const isEntity = typeof resource === 'object'; | ||
| const hasId = isEntity ? !! resource.id : !! id; |
There was a problem hiding this comment.
Feels confusing that we're allowing an ID to be provided as the second argument when working with a resource. This would allow for a hybrid use, like this:
useResourcePermissions( { kind: 'postType', name: 'post' }, 123 )
which feels confusing, since we also allow:
useResourcePermissions( { kind: 'postType', name: 'post', id: 123 } )
since we're defining this behavior right here and right now, does it make sense to remove the ambiguity?
There was a problem hiding this comment.
The hybrid signature won't work. In that case, the hasId would be false; the hook would check general permissions for the REST API and return early.
There was a problem hiding this comment.
Thanks for confirming, but that's even worse. In that case, we need to make sure this behavior is well-documented, and we should probably throw an error if someone calls it with a resource object and a separate ID argument. Because, you know, TS won't be enough sometimes - some projects and developers just don't use it (or abuse it) in their dev environments.
There was a problem hiding this comment.
Throwing an error in a hook might be a bit risky. Depending on where the hook is called in the tree, the closest error boundary might be the one in the app root, preventing the whole app from loading.
I think logging a warning is probably the safest solution here.
There was a problem hiding this comment.
Logging a warning makes sense 👍 I definitely didn't mean throwing a literal error.
There was a problem hiding this comment.
I added the warning using the @wordpress/warning package, which seems to be the new preferred method for such cases.
cc @gziolo
Screenshot
There was a problem hiding this comment.
I’m still processing the usage of warning util as I started to have second thoughts of how we can catch these warnings on production. Do we have to? How folks debug the cases where plugins conflict between each other? My reasoning was that if the app continues to work then we don’t need to add noise to the Browser Console when the app runs on production. I’m happy to discuss it further but for block registration it was a clear path forward as it’s mostly targeting the block author when developing the block.
There was a problem hiding this comment.
In this case, the warning is useful to the developer who's trying to use the API, since it's clear that they're using it in an unexpected way (almost like a front-end counterpart of _doing_it_wrong(). To me, it definitely doesn't make sense to surface that in production, it should only be seen in development mode.
There was a problem hiding this comment.
This warning also has the same target audience - developers. The warnings that aren't actionable by the end user shouldn't be displayed in production environments.
There was a problem hiding this comment.
I think we reached a consensus here. I appreciate the parallel to _doint_it_wrong(). That speaks well to me and maybe we should alias it this way one day to offer more clarity.
Comment on lines
+156
to
+159
| const permissions = useResourcePermissions( { | ||
| kind: 'postType', | ||
| name: postType, | ||
| } ); |
There was a problem hiding this comment.
Nice one 👍 I appreciate the resulting cleanup!
| dispatch.receiveUserPermission( 'read/postType/wp_navigation', allowed ); | ||
| dispatch.startResolution( 'canUser', [ | ||
| 'read', | ||
| { kind: 'postType', name: 'wp_navigation', id: undefined }, |
There was a problem hiding this comment.
Why do we have id: undefined? Shouldn't it just be omitted?
There was a problem hiding this comment.
While selectorArgsToStateKey removes trailing undefined arguments, it doesn't do it inside the arguments themselves.
Omitting id results canUser not marked as resolved.
packages/block-library/src/navigation/test/use-navigation-menu.js
Outdated
Show resolved
Hide resolved
Mamaduka
force-pushed
the
update/use-resource-permissions-hook
branch
from
5c25763
to
9404acb
Compare
ramonjd
changed the title
Mamaduka
force-pushed
the
update/use-resource-permissions-hook
branch
2 times, most recently
from
140d3de
to
6b1ac09
Compare
|
@tyxla, do you think this is in an OK shape to be merged? |
tyxla
approved these changes
Jul 24, 2024
| const create = canUser( | ||
| 'create', | ||
| isEntity | ||
| ? { kind: resource.kind, name: resource.name } |
There was a problem hiding this comment.
Is there a good reason to not trust passing the resource object here directly?
There was a problem hiding this comment.
The resource object can have an id and the hook used to check create actions without IDs. So, one reason is backward compatibility.
Usually, create permissions are checked using the list endpoint OPTIONS v2/pages.
Mamaduka
force-pushed
the
update/use-resource-permissions-hook
branch
from
6b1ac09
to
56f1d2b
Compare
|
Thanks for the review and feedback, @tyxla! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
This is a follow-up to #63322.
PR updates the
useResourcePermissionshook to support new entity-aware syntax. I also replaced usage in the codebase.Why?
The new syntax should be preferred over direct REST API resource queries.
Testing Instructions
Testing Instructions for Keyboard
Same.
Screenshots or screencast