Navigation: Update deps for the useEffect that creates navigation menus #47912
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
6 tasks
|
Size Change: +9.22 kB (+1%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 586ecf4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4133727171
|
scruffian
commented
Feb 9, 2023
| return { | ||
| isSaving: isSavingEntityRecord( 'postType', 'wp_navigation' ), | ||
| draftNavigationMenus: getEntityRecords( | ||
| // This is needed so that hasResolvedDraftNavigationMenus gives the correct status. |
There was a problem hiding this comment.
We have to keep this line, otherwise hasResolvedDraftNavigationMenus doesn't work.
There was a problem hiding this comment.
Can we just not check if navigation menus are resolved? We only need them because
Also ensure other navigation menus have loaded so an
// accurate name can be created.
Is there another way? Like wait for this when creating the default title instead?
There was a problem hiding this comment.
probably but i'm not sure how to do that!
There was a problem hiding this comment.
I don't think so actually - we also need to check if draft navigations are created because navigation we create is a draft. If I remove the check for hasResolvedDraftNavigationMenus then two get created.
There was a problem hiding this comment.
When we call createNavigationMenu we do it with an empty title and there is a hook useGenerateDefaultNavigationTitle where we alreadt check that we loaded all drafts already so we can assign the correct number.
getEntityRecords( ...DRAFT_MENU_PARAMS ),
getEntityRecords( ...PUBLISHED_MENU_PARAMS ),
] );
That means this guard here is kinda pointless.
There was a problem hiding this comment.
Why don't you pass in the isCreatingNavigationMenu status of the createNavigationMenu from the edit component? That way we have a way of telling whether the conversion is in progress. That might help us block the 2nd menu because it gives a true status of the progress of the event we care about - namely: whether we are already creating a navigation.
Relying on various resolved states is surely prone to error.
There was a problem hiding this comment.
That doesn't stop us calling createNavigationMenu twice though does it?
I didn't mean that. I mean that hasResolvedDraftNavigationMenus and hasResolvedNavigationMenus are only needed here as deps because we check for them in the effect - but createNavigationMenu which is the only result of the effect ALSO checks them - it seems they're not needed. And if they're not needed we don't need draftNavigationMenus either.
There was a problem hiding this comment.
If I remove them then the problem comes back, so I think they are needed.
There was a problem hiding this comment.
Why don't you pass in the isCreatingNavigationMenu status of the createNavigationMenu from the edit component?
I don't think this will work. After the new navigation is created, the status is reset to idle, but the block is still dirty as the new (draft) navigation hasn't yet loaded, so the effect gets triggered again, and we end up in a loop creating endless new menus.
There was a problem hiding this comment.
I didn't mean that. I mean that hasResolvedDraftNavigationMenus and hasResolvedNavigationMenus are only needed here as deps because we check for them in the effect - but createNavigationMenu which is the only result of the effect ALSO checks them - it seems they're not needed. And if they're not needed we don't need draftNavigationMenus either.
I don't think this will work, because although createNavigationMenu does check the status of those dependencies, it happens after we call createNavigationMenu, so the useEffect that is checking whether the blocks are in a dirty state will continue to be called.
|
I assume this needs to be backported to WP 6.2; I will update the labels. |
ntsekouras
pushed a commit
that referenced
this pull request
Feb 13, 2023
* Update deps for the useEffect that creates navigation menus * still check for draft menus * remvove unneeded code
|
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: b715cda |
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?
The dependencies in the useEffect should match the variables we use inside the effect. Fixes #47881
Why?
Because otherwise the effect gets called multiple times.
How?
Matching the deps.
Testing Instructions
See #47881.