Skip to content

Fix warnings within useSelect of the bulk editing header #67872

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

Open
louwie17 wants to merge 9 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 all commits
Commits
Show all changes
9 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
ba6868a
Update post/page related functions to return ID as number
louwie17 Dec 17, 2024
9c55bef
Handle post ids as numbers
louwie17 Dec 17, 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
68 changes: 65 additions & 3 deletions packages/core-data/src/private-selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,16 @@ import { createSelector, createRegistrySelector } from '@wordpress/data';
/**
* Internal dependencies
*/
import { getDefaultTemplateId, getEntityRecord, type State } from './selectors';
import {
getDefaultTemplateId,
getEditedEntityRecord,
getEntityRecord,
type State,
type GetRecordsHttpQuery,
} from './selectors';
import { STORE_NAME } from './name';
import { unlock } from './lock-unlock';
import type * as ET from './entity-types';

type EntityRecordKey = string | number;

Expand Down Expand Up @@ -112,7 +119,7 @@ export function getRegisteredPostMeta( state: State, postType: string ) {
return state.registeredPostMeta?.[ postType ] ?? {};
}

function normalizePageId( value: number | string | undefined ): string | null {
function normalizePageId( value: number | string | undefined ): number | null {
if ( ! value || ! [ 'number', 'string' ].includes( typeof value ) ) {
return null;
}
Expand All @@ -122,7 +129,11 @@ function normalizePageId( value: number | string | undefined ): string | null {
return null;
}

return value.toString();
if ( typeof value === 'string' ) {
return parseInt( value, 10 );
}

return value;
}

interface SiteData {
Expand Down Expand Up @@ -257,3 +268,54 @@ export const getTemplateId = createRegistrySelector(
} );
}
);

/**
* 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(
< 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 )
);
},
(
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
]
),
];
}
);
2 changes: 1 addition & 1 deletion packages/core-data/src/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ type Optional< T > = T | undefined;
/**
* HTTP Query parameters sent with the API request to fetch the entity records.
*/
type GetRecordsHttpQuery = Record< string, any >;
export type GetRecordsHttpQuery = Record< string, any >;

/**
* Arguments for EntityRecord selectors.
Expand Down
9 changes: 8 additions & 1 deletion packages/edit-site/src/components/post-edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,14 @@ const fieldsWithBulkEditSupport = [
];

function PostEditForm( { postType, postId } ) {
const ids = useMemo( () => postId.split( ',' ), [ postId ] );
const ids = useMemo(
() =>
postId
.split( ',' )
.map( ( id ) => parseInt( id, 10 ) )
.filter( Number ),
[ postId ]
);
const { record, hasFinishedResolution } = useSelect(
( select ) => {
const args = [ 'postType', postType, ids[ 0 ] ];
Expand Down
47 changes: 34 additions & 13 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 @@ -21,27 +21,48 @@ import { usePostActions } from './actions';
const { Menu, kebabCase } = unlock( componentsPrivateApis );

function useEditedEntityRecordsWithPermissions( postType, postIds ) {
const { items, permissions } = useSelect(
const { items, entityConfig } = useSelect(
( select ) => {
const { getEditedEntityRecord, getEntityRecordPermissions } =
unlock( select( coreStore ) );
const { getEntityConfig } = select( coreStore );
const { getEditedEntityRecords } = unlock( select( coreStore ) );
const entityRecords = getEditedEntityRecords(
'postType',
postType,
postIds
Copy link
Contributor

@talldan talldan Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@louwie17 louwie17 Jan 2, 2025

Choose a reason for hiding this comment

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

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 ).

);

return {
items: postIds.map( ( postId ) =>
getEditedEntityRecord( 'postType', postType, postId )
),
permissions: postIds.map( ( postId ) =>
getEntityRecordPermissions( 'postType', postType, postId )
),
items: entityRecords,
entityConfig: getEntityConfig( 'postType', postType ),
};
},
[ postIds, postType ]
);

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 ]
);
Comment on lines +42 to +57
Copy link
Contributor

@talldan talldan Dec 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@louwie17 louwie17 Jan 2, 2025

Choose a reason for hiding this comment

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

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


return useMemo( () => {
return items.map( ( item, index ) => ( {
...item,
permissions: permissions[ index ],
} ) );
return (
items?.map( ( item, index ) => ( {
...item,
permissions: permissions ? permissions[ index ] : undefined,
} ) ) ?? []
Comment on lines +61 to +64
Copy link
Contributor

@talldan talldan Dec 24, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@louwie17 louwie17 Jan 2, 2025

Choose a reason for hiding this comment

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

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.

);
}, [ items, permissions ] );
}

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

@louwie17 louwie17 Dec 12, 2024

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;

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