-
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
Navigation: Wrap the dependent functions in useCallback #48195
Conversation
Size Change: +23 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
Flaky tests detected in baf23cc. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4231090127
|
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 looks good to me and it fixes a bug where a navigation would be created twice because of repeated firing of effects.
c3ca257
to
baf23cc
Compare
What?
In #48066 we added all the necessary dependencies to the useEffects in the navigation block. Three of these dependencies (handleUpdateMenu, hideNavigationMenuStatusNotice, showNavigationMenuStatusNotice) were functions, which get redefined whenever the component renders. This means we got stuck in a loop where there useEffect would keep firing.
To avoid this we need to wrap these function in
useCallback
which will cache the function until the parameters change. I'm not sure if I have specified the correct dependencies for theuseCallback
functions I have added.Testing Instructions