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

Jump to bottom

Editor: Fix the 'useSelect' warning in the 'useIsDirty' hook #53759

Merged
merged 1 commit into from
Aug 18, 2023
Merged

Editor: Fix the 'useSelect' warning in the 'useIsDirty' hook #53759

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 16 additions & 20 deletions packages/editor/src/components/entities-saved-states/hooks/use-is-dirty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import { useSelect } from '@wordpress/data';
import { store as coreStore } from '@wordpress/core-data';
import { useState } from '@wordpress/element';
import { useMemo, useState } from '@wordpress/element';
import { __ } from '@wordpress/i18n';

const TRANSLATED_SITE_PROPERTIES = {
Expand All @@ -18,38 +18,34 @@ const TRANSLATED_SITE_PROPERTIES = {
};

export const useIsDirty = () => {
const { dirtyEntityRecords } = useSelect( ( select ) => {
const dirtyRecords =
select( coreStore ).__experimentalGetDirtyEntityRecords();
const { editedEntities, siteEdits } = useSelect( ( select ) => {
const { __experimentalGetDirtyEntityRecords, getEntityRecordEdits } =
select( coreStore );

return {
editedEntities: __experimentalGetDirtyEntityRecords(),
siteEdits: getEntityRecordEdits( 'root', 'site' ),
};
}, [] );

const dirtyEntityRecords = useMemo( () => {
// Remove site object and decouple into its edited pieces.
const dirtyRecordsWithoutSite = dirtyRecords.filter(
const editedEntitiesWithoutSite = editedEntities.filter(
( record ) => ! ( record.kind === 'root' && record.name === 'site' )
);

const siteEdits = select( coreStore ).getEntityRecordEdits(
'root',
'site'
);

const siteEditsAsEntities = [];
const editedSiteEntities = [];
for ( const property in siteEdits ) {
siteEditsAsEntities.push( {
editedSiteEntities.push( {
kind: 'root',
name: 'site',
title: TRANSLATED_SITE_PROPERTIES[ property ] || property,
property,
} );
Copy link
Member

Choose a reason for hiding this comment

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

Here it's interesting that we're looking only at the keys of siteEdits, not the values. So, when I'm editing the site title or tagline, then, as i type, the siteEdits change, and trigger a recalculation and rerender. But the keys don't change, only the value.

Fixing this would be a micro-optimization of a very minor use case (editing site properties), probably not worth doing. But interesting to know about 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

So should we just return the record keys for this one? Do we have a data selector to ensure the same value on every render?

P.S. I've another minor refactoring in mind for this hook - using reducer for the unselectedEntities state. Happy to include this micro-optimization there if you want to pursue it.

Copy link
Member

Choose a reason for hiding this comment

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

Now I found that the __experimentalGetDirtyEntityRecords selector has exactly the same problem. But this one is much more widely used. Although its return value is still the same, and although it's memoized with createSelector, the value is recalculated on every keystroke, when changing any block. Because the memoization is invalidated on every content value change, but the content values are not used to calculate the return value.

It won't be easy to memoize this properly.

}
const dirtyRecordsWithSiteItems = [
...dirtyRecordsWithoutSite,
...siteEditsAsEntities,
];

return {
dirtyEntityRecords: dirtyRecordsWithSiteItems,
};
}, [] );
return [ ...editedEntitiesWithoutSite, ...editedSiteEntities ];
}, [ editedEntities, siteEdits ] );

// Unchecked entities to be ignored by save function.
const [ unselectedEntities, _setUnselectedEntities ] = useState( [] );
Expand Down
Loading