-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix warnings within useSelect of the bulk editing header #67872
base: trunk
Are you sure you want to change the base?
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small fix, the getPostsPageId()
and getHomePage()?.postId
actually return strings, so allowSwitchingTemplate
always returned true.
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.
Should we make getHomePage
return the right type?
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.
Meaning an integer? It seems like in some of the other logic you were already depending on it being a string:
postId.toString() === homepage?.postId |
Similar scenario with the
getPostsPageId
that explicitly converts it to a string: function normalizePageId( value: number | string | undefined ): string | null { |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
const homepageId = | |
siteData?.show_on_front === 'page' | |
? normalizePageId( siteData.page_on_front ) | |
: null; | |
if ( homepageId ) { | |
return { postType: 'page', postId: homepageId }; | |
} | |
const frontPageTemplateId = select( | |
STORE_NAME | |
).getDefaultTemplateId( { | |
slug: 'front-page', | |
} ); | |
return { postType: 'wp_template', postId: frontPageTemplateId }; |
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this new selector private for now.
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.
Done: e4cd460
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 :) |
What?
getEditedEntityRecords
which, retrieves multiple edited records.getEntityRecord
request for every record in the list.PostActions
component to usegetEditedEntityRecords
andgetEntityRecordsPermissions
to avoid map usage within useSelect.TemplateEdit
function to not usefilter
withinuseSelect
causing warnings ( breaking memoization ).Why?
Avoids this
useSelect
warning 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 eitheritems
andpermissions
oravailableTemplates
andtemplates
.homepage
page or theposts page
and check if the templates field is either disabled, or doesn't show theSwap template
option. ( You can set thehomepage
andposts page
pages within Settings > Reading >Your homepage displays
Page
tab 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