Fix warnings within useSelect of the bulk editing header #67872
Conversation
louwie17
added
the
[Feature] DataViews
louwie17
requested review from
ajitbohra,
fabiankaegy,
juanmaguitar,
ndiego 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. |
packages/core-data/src/selectors.ts
Outdated
| recordIds: EntityRecordKey[] | ||
| ): Array< ET.Updatable< EntityRecord > | false > => { | ||
| return recordIds.map( ( recordId ) => | ||
| getEditedEntityRecord( state, kind, name, recordId ) |
There was a problem hiding this comment.
Given I re-use getEditedEntityRecord, it would trigger the getEntityRecord resolver for every record id in the list.
@youknowriad or @oandregal do you know why this happens when we already technically got all the records in the able using useEntityRecords. Is it because we are not triggering a finishResolution for each record as part of that call?
There was a problem hiding this comment.
Is it because we are not triggering a finishResolution for each record as part of that call?
That's probably true.
What are our options? Should we use getEntityRecords instead of getEntityRecord in getEditedEntityRecords
There was a problem hiding this comment.
Actually I stand corrected, the finishResolution does work correctly. The requests I am seeing are OPTIONS calls triggered by the canUser selector for getting the permissions: https://github.com/WordPress/gutenberg/blob/trunk/packages/core-data/src/resolvers.js#L524-L533
There was a problem hiding this comment.
We can run permissions selector after items are resolved. This way we can avoid dispatching a permission request for each item.
There was a problem hiding this comment.
I can try, but I am skeptical that would work. Given the individual record calls are not triggered because they are resolved by the getEntityRecords call.
Cause would it be enough to check permissions just for the postType? or stick with checking for every record.
There was a problem hiding this comment.
Ok, so the problem was that I was passing the postIds in as strings into the permission call which re-triggered the requests. Although it had already been resolved prior by the data for the DataViews list.
I updated the above in this commit: 6972c79 to follow the same format as this hook: https://github.com/WordPress/gutenberg/blob/trunk/packages/core-data/src/hooks/use-entity-records.ts#L158-L204
Where we get the records first, and then get the list of ids from the actual records ( instead of relying on the parsed ids ). This seems to fix the trigger for additional OPTIONS request.
There was a problem hiding this comment.
It should. The getEntityRecords selector will try to resolve permissions for each entity when possible (ref #64504). However, we had a race condition because it called both selectors simultaneously.
Ok, so the problem was that I was passing the postIds in as strings into the permission call which re-triggered the requests.
Maybe we should normalize IDs before passing them to selectors.
There was a problem hiding this comment.
Maybe we should normalize IDs before passing them to selectors.
Yeah not a bad plan, with the current changes I suppose we are doing a form of normalizing by relying on the ID type that the entity record has.
| ).getEntityRecords< WpTemplate >( 'postType', 'wp_template', { | ||
| per_page: -1, | ||
| post_type: postType, | ||
| } ); |
There was a problem hiding this comment.
Remove the ?? [] and handle this outside of the useSelect.
| postType === 'page' && getHomePage()?.postId === +postId; | ||
|
|
||
| const allowSwitchingTemplate = ! isPostsPage && ! isFrontPage; | ||
| postType === 'page' && +getHomePage()?.postId === postId; |
There was a problem hiding this comment.
Small fix, the getPostsPageId() and getHomePage()?.postId actually return strings, so allowSwitchingTemplate always returned true.
There was a problem hiding this comment.
Should we make getHomePage return the right type?
There was a problem hiding this comment.
Meaning an integer? It seems like in some of the other logic you were already depending on it being a string:
Similar scenario with the
getPostsPageId that explicitly converts it to a string:
Instead of converting it to an integer above, maybe I should just keep the postId as a string?
There was a problem hiding this comment.
If I'm not wrong, the "posts" REST API returns post Ids as integers, except for templates and template Parts. So IMO, we should stay consistent with that. When the post type is "template" or "template part" use string ids, otherwise integer ids.
There might be some type casting we need to do at the route levels, because urls always give strings.
WDYT?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah I agree we should stay consistent, and happy to stick with When the post type is "template" or "template part" use string ids, otherwise integer ids..
Would it help to have a sanitize function for this that includes the postType and uses the above logic?
Things may get a bit confusing in functions like getHomePage (
gutenberg/packages/core-data/src/private-selectors.ts
Lines 144 to 156 in 4867dfc
page or wp_template so the postId will likewise be either a string or integer. Which in a way I see why you piped it through normalizePageId to convert it to a string.
|
Size Change: +151 B (+0.01%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
packages/core-data/src/selectors.ts
Outdated
| * | ||
| * @return The list of entity records, merged with their edits. | ||
| */ | ||
| export const getEditedEntityRecords = createSelector( |
There was a problem hiding this comment.
Let's make this new selector private for now.
|
Flaky tests detected in e4cd460. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12300636881
|
|
@Mamaduka & @youknowriad this is ready for a re-review. I moved the selector to be private and updated the |
It fixed the multiple I suppose I can move pretty well everything into a single |
I was not able to move everything into a single |
|
@Mamaduka for when you have some time, this PR is ready for a re-review :) |
| const ids = useMemo( | ||
| () => | ||
| items?.map( ( record ) => record[ entityConfig?.key ?? 'id' ] ) ?? | ||
| [], | ||
| [ items, entityConfig?.key ] | ||
| ); | ||
|
|
||
| const permissions = useSelect( | ||
| ( select ) => { | ||
| const { getEntityRecordsPermissions } = unlock( | ||
| select( coreStore ) | ||
| ); | ||
| return getEntityRecordsPermissions( 'postType', postType, ids ); | ||
| }, | ||
| [ ids, postType ] | ||
| ); |
There was a problem hiding this comment.
This is a bit of a drive-by comment, and I don't know much about the code, so I might be far off the mark.
Can getEntityRecordsPermissions do this whole dance with the keys internally?
It seems inconsistent that it's possible to call getEditedEntityRecords with the postIds, but for getEntityRecordsPermissions the postIds have to be mapped to a particular key. It might also be a gotcha for any users of the API.
There was a problem hiding this comment.
Can getEntityRecordsPermissions do this whole dance with the keys internally?
It certainly could, I think we did have to create a separate selector to handle that.
I currently followed the same format as an already existing hook: https://github.com/WordPress/gutenberg/blob/trunk/packages/core-data/src/hooks/use-entity-records.ts#L158-L204
It seems inconsistent that it's possible to call getEditedEntityRecords with the postIds, but for getEntityRecordsPermissions the postIds have to be mapped to a particular key. It might also be a gotcha for any users of the API.
Yeah, I can probably remove it now given I explicitly converted the postIds to integers upstream: https://github.com/WordPress/gutenberg/pull/67872/files/9c55bef36e3190e74e5864f0b3a56989b12f7509#diff-4ed486e973856f952d2b80aea381fc1f5a1d0004995a0b68554147b02cee0b55R36-R43
| items?.map( ( item, index ) => ( { | ||
| ...item, | ||
| permissions: permissions ? permissions[ index ] : undefined, | ||
| } ) ) ?? [] |
There was a problem hiding this comment.
Again I'll leave a caveat that I don't know the code very well, but I feel a bit nervous about the way the permissions are merged into the entity records here.
It might be better if this was TypeScript, but because it's JS, other contributors will have to trace back through the code to find out whether the items contain the permissions key or not. It's the kind of thing that could lead to bugs if another contributor makes a bad assumption.
I think it's clearer and more intentional if they're kept separate, and also the hook ends up returning a more pure form of getEntityRecordPermissions, which feels more elegant to me. I think it could simplify the code quite a bit.
There was a problem hiding this comment.
It might be better if this was TypeScript, but because it's JS, other contributors will have to trace back through the code to find out whether the items contain the permissions key or not. It's the kind of thing that could lead to bugs if another contributor makes a bad assumption.
I don't know if I totally agree with this, given we explicitly set this list to the itemsWithPermissions constant, which should give enough context.
Having said that, I do agree that keeping them separate would be ideal. This would mean either updating the isEligible ( link ) function to take permissions, or push consumers to handle getting permissions themselves within the isEligible function. This may mean we should support it being async as well.
@youknowriad or @oandregal curious if you have an opinion here?
I would lean towards the former, where we may add an additional context param to the isEligible function that may contain permissions.
Advocating for a context param, as permissions is post specific and would not make sense for some of the other use cases.
| const entityRecords = getEditedEntityRecords( | ||
| 'postType', | ||
| postType, | ||
| postIds |
There was a problem hiding this comment.
At the start of PostActions, there's a useMemo call (I can't leave a code comment there as it's not part of the changeset):
const _postIds = useMemo( () => {
if ( Array.isArray( postId ) ) {
return postId;
}
return postId ? [ postId ] : [];
}, [ postId ] );I think that's unnecessary overhead, the coercion to an array can be performed in this useSelect within useEditedEntityRecordsWithPermissions. From what I could tell useEditedEntityRecordsWithPermissions is the only place _postIds is used, so I think it's a good idea to move that code inside this hook.
There was a problem hiding this comment.
Yeah that is also an option, although if we move the useMemo code into the useSelect it would mean we pass a new array of ids into getEditedEntityRecords which in turn will trigger the memoization of the getEditedEntityRecords selector and cause the same warning to appear that this PR attempts to prevent ( when handling single ids ).
|
I'm considering reverting the bulk actions from the Quick Edit panel to avoid accidentally leaking this issue in the next major WP release, especially since Quick Edit for DataViews is still experimental. I see this as an opportunity to re-evaluate bulk edits at a lower level. Our current hacks in the codebase don't scale well and add developer overhead.
@louwie17, thanks again for contributing, and hopefully, we can resume work on this enhancement shortly. |
What exactly are you considering reverting since the whole panel is experimental so unless I'm missing something is not going to leak? |
|
Probably #67743 and #67390. I haven't started working on them yet, but I will try to keep the changes to a minimum.
The |
I think making PostActions component support bulk actions is a good thing, I don't really see why we'd need to revert that personally. Your call though. |
Doing something similar as this makes a lot more sense IMO. Cause is it actually possible to have different permissions for specific posts or pages? For example could a user have edit/delete permissions on one post and not the other? Is this something we already have a data store selector for, or more generally an API that provides this on a post type basis?
Are we not already using this? Currently the bulk editing is handled locally and the user has to trigger the save afterwards, that is all handled by the entity data store ( as far as I am aware ). This could definitely use the batch approach if there are multiple records that need saving.
👍
Let me know if you need a review on this. I believe the |
|
This PR still looks like it's under discussion, but I think at least the browser warning should be fixed in the WP 6.8 release. I'll add this PR to the project board so we don't forget to take action. |
Hey @Mamaduka, reverting some of those changes would take a bit of time, which I unfortunately don't have at the moment either. |
|
Thanks for the reply, @louwie17! Yeah, right now, it's impossible to clean revert using Git, so it should be a manual effort. I think this PR still needs polishing. Here's a small example: The Also, there's always a chance we discover minor issues with this feature, and I'm not confident in our current resources to follow up properly in time. |
|
Here's the revert PR - #69341. |
What?
getEditedEntityRecordswhich, retrieves multiple edited records.getEntityRecordrequest for every record in the list.PostActionscomponent to usegetEditedEntityRecordsandgetEntityRecordsPermissionsto avoid map usage within useSelect.TemplateEditfunction to not usefilterwithinuseSelectcausing warnings ( breaking memoization ).Why?
Avoids this
useSelectwarning as pointed out in this comment: #67743 (comment) ( same goes for the template field ):How?
Removing usage of
map,filter, and empty array within auseSelect.Testing Instructions
The 'useSelect; hook returns different values when called with the same state and parameters. This can lead to unnecessary re-renders and performance issues if not.is not being triggered for eitheritemsandpermissionsoravailableTemplatesandtemplates.homepagepage or theposts pageand check if the templates field is either disabled, or doesn't show theSwap templateoption. ( You can set thehomepageandposts pagepages within Settings > Reading >Your homepage displays
Pagetab in the editing settings menu ( on the far right )Testing Instructions for Keyboard
Screenshots or screencast
Before
bulk-editing-with-warnings.mp4
After
bulk-editing-no-warnings.mp4