Fix warnings within useSelect of the bulk editing header #67872
Conversation
louwie17
added
the
[Feature] DataViews
louwie17
requested review from
nerrad,
ajitbohra,
juanmaguitar,
fabiankaegy and
ndiego
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.
| 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.
| 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.
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