-
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
Block bindings: Improve performance by memo postypes slug. #65705
Conversation
const postTypesSlugs = useMemo( () => { | ||
return ( | ||
postTypes?.map( ( entity ) => entity.slug ) || [] | ||
); | ||
}, [] ); |
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.
Using hooks inside condition is against React Hook rules, and I don't think nesting useMemo
calls is a good idea.
We should move postTypesSlugs
map outside of current memo function.
Size Change: +146 B (+0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
It seems that specific metric varies a lot. Taking a look other random PRs: I'm more worried about Taking a look at the PRs that got merged before that one, it seems it could affect that.
According to this data, it is a possibility that the original PR introduced a regression on this aspect. I'd like to:
Taking a look at the performance metrics from this PR, it seems the |
I've opened this other pull request to potentially palliate the |
@SantosGuillamot, can we close this PR now that #66101 is merged? |
The biggest performance impact seems to be solved after that PR, yes. Having said that, if you think the logic in this pull request can still help, I'm fine with it. I don't have much experience with this topic, so I don't have a strong opinion. |
Closing in favor of #66101 |
What?
Improve the perfomance of the postTypesSlugs by memoizing it.
trunk:
PR: