-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Patterns: move mapping of values from core-data selector into consumers #54576
Conversation
Size Change: +47 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
Flaky tests detected in 57d6950. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6230511039
|
packages/edit-site/src/components/page-patterns/use-patterns.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-pattern/use-pattern-details.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Kai Hao <kevin830726@gmail.com>
Co-authored-by: Kai Hao <kevin830726@gmail.com>
Co-authored-by: Kai Hao <kevin830726@gmail.com>
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 for updating the approach here @glendaviesnz 👍
It tests well for me. I didn't spot any regressions in the post editor inserter, its pattern explorer, or in the site editor's pattern management page.
Looks like you've already fixed the only tweaks I was going to suggest. Nice work!
It might be worth noting #54580 as a related element to the performance issue that was noted in this PR description.
Done. |
const categories = new Map(); | ||
userPatternCategories.forEach( ( userCategory ) => | ||
categories.set( userCategory.id, userCategory ) | ||
); |
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.
This will still return a new value every time mapSelect
is called. You should see the same useSelect
notice where this hook is used.
We should generally avoid mapping/grouping the items inside the selectors to avoid bugs like #53596.
P.S. The usePatterns
already had this issue, and I've left a comment on the original PR. Cc @kevin940726.
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.
Appreciate you following up on the useSelect notices here @Mamaduka 👍
I was hoping to have a look into these this afternoon.
Fixing up use-pattern-details.js
is pretty straightforward, it might take me a bit longer to untangle the usePatterns
hook though.
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.
@aaronrobertshaw, yes, the usePatterns
will take more refactoring to untangle :(
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'll throw up a PR for usePatternDetails
shortly so that small chunk isn't held up by the larger refactor.
Edit: Initial PR is available - #54584
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 @Mamaduka! I knew that there was a ping from you but I couldn't remember where 😅.
Yeah, the use-patterns
one was my bad, but I don't think it's the bottleneck in most cases. Re-renders aren't bad IMO, only when the computation is heavy.
However, I agree we can optimize it, usually done by memoizing intermediate results. I'm looking into ways to memoize the selected data using rememo
and let's see if it helps!
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.
@kevin940726 is already working on usePatterns
so I'll aim to help him on that front.
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.
Thank you both! Happy to help out with testing/reviews.
Thanks for the follow-ups, @glendaviesnz! This solves the issue with |
@Mamaduka thanks for the follow-up, I think I was juggling too many balls today trying to solve the main typing regression so missed that remaining mapSelect issue. Detailed changes to editor store selectors is a bit of a new area for me so still getting to grips with what ideally should happen where - so I really appreciate your recent pointers around this! I think it is all much clearer now, so can hopefully avoid this sort of mistake in future. |
@glendaviesnz, no worries at all, and thanks again for the quick follow-ups! |
What?
Moves mapping of values from core-data selector into consumers.
Why?
The introduction of the user pattern categories has caused a decrease in typing performance in the post editor, so this PR is cleaning up a selector that was added as part of this merge in an attempt to improve the typing performance again.
How?
Removes the creation of the pattern categories by id for the selector, and instead create this map in the consumers of the selector.
While investigating this issue another similar performance issue that could affect typing was found in the useBlockEditorSettings hook and this was fixed in #54580.
Testing Instructions
Screenshots
patterns-performance-update.mp4