Memoize useSelect for usePatterns
#54588
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +88 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
kevin940726
force-pushed
the
fix/memoize-use-patterns
branch
from
cf61ba4
to
1a6c423
Compare
kevin940726
requested review from
Mamaduka,
glendaviesnz and
aaronrobertshaw
|
@Mamaduka Do you want to give it a quick test to see if it works for you? 🙇 |
There was a problem hiding this comment.
Thanks for tackling this one @kevin940726 👍
I didn't spot any issues or regressions while exploring the pattern management page in the Site Editor. Whether it was navigating categories, searching, filtering, viewing, editing or creating patterns it all seemed to work well. No warnings or errors appeared in devtools console.
I'm not very familiar with rememo so it would be great to get some extra eyes on this and sanity check the approach.
From my end though, the changes make sense and avoid a more drastic refactor, while still addressing the console warnings.
Nice work ✨
Mamaduka
reviewed
Sep 19, 2023
|
Thank you, @kevin940726! I'll have a more in-depth look as soon as possible. |
kevin940726
force-pushed
the
fix/memoize-use-patterns
branch
from
0b0dd30
to
65e310d
Compare
kevin940726
force-pushed
the
fix/memoize-use-patterns
branch
from
65e310d
to
381abf6
Compare
kevin940726
added
the
[Feature] Patterns
Mamaduka
approved these changes
Sep 22, 2023
There was a problem hiding this comment.
The code looks good to me, but using the createSelector like this feels bit odd 😅
Ideally, we would return only selector values from useSelect and handle filtering/mapping outside of the hook, but I understand this would require langer refactoring.
I'm okay with landing this solution and then following up with refactoring. What do you think?
P.S. This can count as a bug fix or code quality improvement. I think it would be okay to land in Gutenberg RC.
Yeah, it's unconventional, but it's also essentially how selectors work. I agree we should follow the convention as much as possible by placing the selectors in a Thanks for reviewing! We can work on refactoring them into conventional selectors next! ❤️ |
kevin940726
added
the
Backport to Gutenberg RC
mikachan
added a commit
that referenced
this pull request
Sep 22, 2023
mikachan
removed
the
Backport to Gutenberg RC
|
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: ab08f21 |
mikachan
added
the
Backport to Gutenberg RC
mikachan
pushed a commit
that referenced
this pull request
Sep 25, 2023
|
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: bbdcba0 |
mikachan
removed
the
Backport to Gutenberg RC
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Originally reported in #52303 (comment) by @Mamaduka. This PR memoizes some selectors to avoid re-renders.
It's best to be reviewed with whitespaces hidden.
Why?
Even though re-renders aren't necessarily bad, and sometimes over-memoizing selectors comes with a cost, I don't think memoizing it brings any harm either. It silences a warning in console so that must be good! 😆
How?
Use
rememoto memoize some selectors used inusePatterns.Testing Instructions
usePatterns(site-editor.php?path=/patterns) and navigate around or interact with it.useSelectwarning aboutusePatterns.Testing Instructions for Keyboard
Same as above.
Screenshots or screencast
N/A