-
Notifications
You must be signed in to change notification settings - Fork 818
Jetpack: WordPress 6.8 Compatibility - Fix data manipulation within useSelect in core features #42435
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
Jetpack: WordPress 6.8 Compatibility - Fix data manipulation within useSelect in core features #42435
Changes from 9 commits
e071726
ac4e43d
4133fc8
01aa10f
f4ca5c6
1fcef0b
d344498
c956a65
96ba631
66ace76
6b65559
544d41f
143e9cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
Significance: patch | ||
Type: other | ||
|
||
Compatibility: Ensuring performance best practices and reducing console warnings. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -192,14 +192,35 @@ export const SlideshowEdit = ( { | |
); | ||
}; | ||
|
||
const memoCache = new Map(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering whether we should clear the map somewhere, maybe when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good thought, thank you. I looked into this more, and from what I understand it would be overkill based on page refresh that should clean this up so the cache is per session, and that the data is lightweight (only image id and size information, not remaining image information). And lastly in attempting to add this it was turning into a complex operation as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely! Thanks for looking it up. |
||
|
||
export default compose( | ||
withSelect( ( select, props ) => { | ||
const imageSizes = select( 'core/editor' ).getEditorSettings().imageSizes; | ||
const resizedImages = props.attributes.ids.reduce( ( currentResizedImages, id ) => { | ||
const image = select( 'core' ).getMedia( id ); | ||
const { getEditorSettings } = select( 'core/editor' ); | ||
const { ids } = props.attributes; | ||
|
||
const imageSizes = getEditorSettings().imageSizes; | ||
|
||
// Create cache key | ||
const memoKey = ids.join( ',' ); | ||
|
||
if ( memoCache.has( memoKey ) ) { | ||
return { | ||
imageSizes, | ||
resizedImages: memoCache.get( memoKey ), | ||
}; | ||
} | ||
|
||
// If not in cache, calculate new value | ||
const { getMedia } = select( 'core' ); | ||
const resizedImages = ids.reduce( ( currentResizedImages, id ) => { | ||
const image = getMedia( id ); | ||
const sizes = get( image, [ 'media_details', 'sizes' ] ); | ||
return [ ...currentResizedImages, { id, sizes } ]; | ||
}, [] ); | ||
|
||
memoCache.set( memoKey, resizedImages ); | ||
|
||
return { | ||
imageSizes, | ||
resizedImages, | ||
|
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.
Could that condition be moved out of the function passed to
useSelect
? Or would that create an issue?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.
So,
useSelect
itself can't be called conditionally. I believe we could miss important store updates when switching between states as well if we were able to use it conditionally.