Preload request for fallback Navigation menu in Site Editor #50545
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
getdave
added
[Type] Performance
|
Size Change: +2 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
Thanks for continuing to find performance optimization opportunities 🙌
For #48683 I'd say it was easy enough to weigh the tradeoff - most themes and most sites would have some sort of navigation, so the benefit is clear. In this case, though, I'm not sure it's justified - if half of the page loads the optimization is a burden and the other half it's an improvement, is it really a worthy optimization? I realize it's hard to judge but I wanted to see what you think about the tradeoffs and how you determine whether it's not just a zero-sum game (or worse). |
|
Yes after some discussion with @scruffian away from Github I'm leaning towards not preloading this request. Whilst it's useful for a specific set of users when they start a brand new site, it will slow things for a large number of sites where that is not the case. As you say it's difficult to weigh that trade off but I feel like we should be cautious of placing to much burden on preloading everything. @draganescu I know you were originally keen on the approach in this PR. Based on the discussion about how do you feel about it now? |
|
Well given that once there is a ref in the block we don't need the fallback we could say preloading is too big of a gun to use here. However issues like #50483 will require us to always fire it because in the navigation screen of the site editor sidebar we don't have access to a ref. So we're back to always pinging this endpoint. |
|
The plan for Navigation on Browse Mode is to list all navigation menus that a user has. This will mean always making a request to fetch all navigations, which might sometimes return nothing. What do you think about also modifying the endpoint that fetches all navigations to call the fallback logic in the case where there are no navigation CPTs? That way we wouldn't need to preload the fallback endpoint because we would have already created and returned the fallback. |
I think we discussed in another context previously and decided that modifying the behaviour of Core endpoints in a way that is unexpected goes against the rules of the WP REST API. This applies even if we're using filters to achieve the result. |
|
I think we should not preload. Rather I think we should simply always show the That way by the time someone clicks on the |
|
Ok that makes sense to me, shall we close this then? |
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?
Extends #48683 by also preloading the request for the fallback Navigation menu.
Why?
Calls to the fallback endpoint slow the rendering of the Nav block (or anything that uses the fallback endpoint). By preloading we avoid a network fetch on the client side.
This is more controversial than #48683 because the fallbacks endpoint isn't always triggered. It's a great optimisation for the scenarios outlined below but otherwise is a waste of resources for a request that doesn't need to be made.
Not sure how we weigh that tradeoff.
The fallback request will be triggered when the block is "empty" (i.e. has no
refattribute). This meansIn the future this fallback request may also be triggered by the
Navigationsection of the Browse Mode sidebar.How?
Preloads the matching request.
Testing Instructions
On TRUNK
navigation-fallbackendpoint.On this PR
navigation-fallbackendpoint.Testing Instructions for Keyboard
Screenshots or screencast