Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from
Open

Fix warnings within useSelect of the bulk editing header #67872

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
aec7269
Add getEditedEntityRecords selector
louwie17 Dec 12, 2024
2840862
Remove map usage from useSelect
louwie17 Dec 12, 2024
5762893
Remove filter usage from template useSelect
louwie17 Dec 12, 2024
6972c79
Update hook to use correct ids for permissions
louwie17 Dec 12, 2024
fce61de
Compare using strings for template id
louwie17 Dec 12, 2024
e4cd460
Move new selector to private
louwie17 Dec 12, 2024
9e4ceb6
Move getEntityConfig inside of other useSelect
louwie17 Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions docs/reference-guides/data/data-core.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,21 @@ _Returns_

- `ET.Updatable< EntityRecord > | false`: The entity record, merged with its edits.

### getEditedEntityRecords

Returns a list of entity records, merged with their edits.

_Parameters_

- _state_ `State`: State tree.
- _kind_ `string`: Entity kind.
- _name_ `string`: Entity name.
- _recordIds_ `EntityRecordKey[]`: Record IDs.

_Returns_

- `Array< ET.Updatable< EntityRecord > | false >`: The list of entity records, merged with their edits.

### getEmbedPreview

Returns the embed preview for the given URL.
Expand Down
15 changes: 15 additions & 0 deletions packages/core-data/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,21 @@ _Returns_

- `ET.Updatable< EntityRecord > | false`: The entity record, merged with its edits.

### getEditedEntityRecords

Returns a list of entity records, merged with their edits.

_Parameters_

- _state_ `State`: State tree.
- _kind_ `string`: Entity kind.
- _name_ `string`: Entity name.
- _recordIds_ `EntityRecordKey[]`: Record IDs.

_Returns_

- `Array< ET.Updatable< EntityRecord > | false >`: The list of entity records, merged with their edits.

### getEmbedPreview

Returns the embed preview for the given URL.
Expand Down
51 changes: 51 additions & 0 deletions packages/core-data/src/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,57 @@ export const getEditedEntityRecord = createSelector(
}
);

/**
* Returns a list of entity records, merged with their edits.
*
* @param state State tree.
* @param kind Entity kind.
* @param name Entity name.
* @param recordIds Record IDs.
*
* @return The list of entity records, merged with their edits.
*/
export const getEditedEntityRecords = createSelector(
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: e4cd460

< EntityRecord extends ET.EntityRecord< any > >(
state: State,
kind: string,
name: string,
recordIds: EntityRecordKey[]
): Array< ET.Updatable< EntityRecord > | false > => {
return recordIds.map( ( recordId ) =>
getEditedEntityRecord( state, kind, name, recordId )
Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Member

@Mamaduka Mamaduka Dec 12, 2024 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

);
},
(
state: State,
kind: string,
name: string,
recordIds: EntityRecordKey[],
query?: GetRecordsHttpQuery
) => {
const context = query?.context ?? 'default';
return [
state.entities.config,
...recordIds.map(
( recordId ) =>
state.entities.records?.[ kind ]?.[ name ]?.queriedData
.items[ context ]?.[ recordId ]
),
...recordIds.map(
( recordId ) =>
state.entities.records?.[ kind ]?.[ name ]?.queriedData
.itemIsComplete[ context ]?.[ recordId ]
),
...recordIds.map(
( recordId ) =>
state.entities.records?.[ kind ]?.[ name ]?.edits?.[
recordId
]
),
];
}
);

/**
* Returns true if the specified entity record is autosaving, and false otherwise.
*
Expand Down
12 changes: 6 additions & 6 deletions packages/editor/src/components/post-actions/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@ const { Menu, kebabCase } = unlock( componentsPrivateApis );
function useEditedEntityRecordsWithPermissions( postType, postIds ) {
const { items, permissions } = useSelect(
( select ) => {
const { getEditedEntityRecord, getEntityRecordPermissions } =
const { getEditedEntityRecords, getEntityRecordsPermissions } =
unlock( select( coreStore ) );
return {
items: postIds.map( ( postId ) =>
getEditedEntityRecord( 'postType', postType, postId )
),
permissions: postIds.map( ( postId ) =>
getEntityRecordPermissions( 'postType', postType, postId )
items: getEditedEntityRecords( 'postType', postType, postIds ),
permissions: getEntityRecordsPermissions(
'postType',
postType,
postIds
),
};
},
Expand Down
79 changes: 41 additions & 38 deletions packages/fields/src/fields/template/template-edit.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,52 +39,49 @@ export const TemplateEdit = ( {
typeof data.id === 'number' ? data.id : parseInt( data.id, 10 );
const slug = data.slug;

const { availableTemplates, templates } = useSelect(
const { templates, allowSwitchingTemplate } = useSelect(
( select ) => {
const allTemplates =
select( coreStore ).getEntityRecords< WpTemplate >(
'postType',
'wp_template',
{
per_page: -1,
post_type: postType,
}
) ?? [];
const allTemplates = select(
coreStore
).getEntityRecords< WpTemplate >( 'postType', 'wp_template', {
per_page: -1,
post_type: postType,
} );
Copy link
Contributor Author

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.


const { getHomePage, getPostsPageId } = unlock(
select( coreStore )
);

const isPostsPage = getPostsPageId() === +postId;
const isPostsPage = +getPostsPageId() === postId;
const isFrontPage =
postType === 'page' && getHomePage()?.postId === +postId;

const allowSwitchingTemplate = ! isPostsPage && ! isFrontPage;
postType === 'page' && +getHomePage()?.postId === postId;
Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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:

gutenberg/packages/core-data/src/private-selectors.ts

Line 188 in 4867dfc

postId.toString() === homepage?.postId

Similar scenario with the getPostsPageId that explicitly converts it to a string:

gutenberg/packages/core-data/src/private-selectors.ts

Line 115 in 4867dfc

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?

Copy link
Contributor

@youknowriad youknowriad Dec 13, 2024 •
edited
Loading

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.

Sign in to view

This comment was marked as outdated.

Sign in to view

Copy link
Contributor Author

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 };
) where it returns either a 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.


return {
templates: allTemplates,
availableTemplates: allowSwitchingTemplate
? allTemplates.filter(
( template ) =>
template.is_custom &&
template.slug !== data.template &&
!! template.content.raw // Skip empty templates.
)
: [],
allowSwitchingTemplate: ! isPostsPage && ! isFrontPage,
};
},
[ data.template, postId, postType ]
[ postId, postType ]
);

const templatesAsPatterns = useMemo(
() =>
availableTemplates.map( ( template ) => ( {
name: template.slug,
blocks: parse( template.content.raw ),
title: decodeEntities( template.title.rendered ),
id: template.id,
} ) ),
[ availableTemplates ]
allowSwitchingTemplate
? ( templates ?? [] )
.filter(
( template ) =>
template.is_custom &&
template.slug !== data.template &&
!! template.content.raw // Skip empty templates.
)
.map( ( template ) => ( {
name: template.slug,
blocks: parse( template.content.raw ),
title: decodeEntities( template.title.rendered ),
id: template.id,
} ) )
: [],
[ templates, allowSwitchingTemplate, data.template ]
);

const shownTemplates = useAsyncList( templatesAsPatterns );
Expand Down Expand Up @@ -151,6 +148,10 @@ export const TemplateEdit = ( {
variant="tertiary"
size="compact"
onClick={ onToggle }
accessibleWhenDisabled
disabled={
value === '' && ! templatesAsPatterns.length
}
>
{ currentTemplate
? getItemTitle( currentTemplate )
Expand All @@ -159,14 +160,16 @@ export const TemplateEdit = ( {
) }
renderContent={ ( { onToggle } ) => (
<MenuGroup>
<MenuItem
onClick={ () => {
setShowModal( true );
onToggle();
} }
>
{ __( 'Swap template' ) }
</MenuItem>
{ templatesAsPatterns.length > 0 && (
<MenuItem
onClick={ () => {
setShowModal( true );
onToggle();
} }
>
{ __( 'Swap template' ) }
</MenuItem>
) }
{
// The default template in a post is indicated by an empty string
value !== '' && (
Expand Down
Loading