-
Notifications
You must be signed in to change notification settings - Fork 816
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
Jetpack: WordPress 6.8 Compatibility - Fix data manipulation within useSelect in core features #42435
Conversation
… in usePromptTags
… in useRelatedPosts
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 4 files.
Full summary · PHP report · JS report Coverage check overridden by
I don't care about code coverage for this PR
|
… in ProductManagementToolbarControl
… in usePassthroughAttributes
// Get tags with memoized query | ||
const tags = useSelect( | ||
select => { | ||
if ( tagIds.length === 0 ) { |
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.
@@ -192,14 +192,35 @@ export const SlideshowEdit = ( { | |||
); | |||
}; | |||
|
|||
const memoCache = new Map(); |
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'm wondering whether we should clear the map somewhere, maybe when SlideshowEdit
unmounts. But that may be overkill.
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.
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.
As for page refresh cleaning the data up, I could confirm this by adding window.debugMemoCache = memoCache;
to the code, then logging this in the browser console before adding a block with images, and then after a page refresh. After the page refresh it defaults back to a size of 1. And if I remove the Slideshow ad refresh, the cache is gone.
So based on that I think most likely overkill here, would you agree?
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.
Absolutely! Thanks for looking it up.
… most current data when updating
…tiple functions from it
Thanks for the reviews @monsieur-z ! Separately I realized there were a couple of other small improvements I could make, explained in the additional commit messages, so I've just pushed those changes now too. |
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.
Thanks @monsieur-z ! Yes that one is dealt with in a separate PR here: #42536 |
I'm seeing these warnings in the console on Dotcom:
Could it be related to this PR? |
Or, maybe you just fixed that haha :D |
Fixes #42434
Proposed changes:
useSelect
calls. This prevents console warnings in the upcoming WordPress 6.8 release, if script debug is enabled.useMemo
prevents unnecessary object creation, and only recalculates when dependencies actually change (usePassthroughAttributes
)ProductManagementToolbarControl
returns the selector's value directly without wrapping it in an object and only changes whengetSelectedProducts
returns a new object.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
On a self-hosted test site, to reproduce the issues on trunk:
define( 'SCRIPT_DEBUG', true );
inwp-config.php
.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 fixed.
tags
, and the Error Component Stack begins withBloggingPromptEdit
(note - the red tags post request console warning occurs on trunk as well, unrelated)selectedProducts
, and the Error Component Stack begins withProductManagementToolbarControl
. The button block issue also visible with the Payments button block, has a non-equal value key ofattributesToSync
.resizedImages
.To test the fix:
You can also test on WoA and Simple (without testing WP 6.8) to ensure no issues with the blocks.