-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: preload more API requests #35402
Navigation: preload more API requests #35402
Conversation
Size Change: -4.04 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
We still have 1 XHR request left because we can only use data from the cache once. |
Why are we fetching data a second time? |
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.
Code LGTM and I can see only one XHR request in DevTools when I browse to the navigation screen 👍
// Unsetting the cache key ensures that the data is only preloaded a single time | ||
const cacheData = cache[ method ][ path ]; | ||
delete cache[ method ][ path ]; |
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.
Tiny nit: would be nice to format this similarly to the above branch
// Unsetting the cache key ensures that the data is only preloaded a single time | |
const cacheData = cache[ method ][ path ]; | |
delete cache[ method ][ path ]; | |
const cacheData = cache[ method ][ path ]; | |
// Unsetting the cache key ensures that the data is only preloaded a single time | |
delete cache[ method ][ path ]; | |
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 agree.
Fixed in d71c48f
array( | ||
'per_page' => $results_per_page, | ||
'context' => 'edit', | ||
'_locale' => 'user', |
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.
Is this needed?
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.
Yes, this is needed.
Cache records are matched by the path.
If we don't use this path, we will not be able to find the cache record.
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 don't like how the path must be kept in sync between two separate places (JS and PHP), but I also don't have any better ideas to offer. 😒
How does this handle the fact the REST API preloaded, means that caches have to be bust. See #24642 (comment) |
Thank you for the review. |
By cache, I mean the preloading itself. That is a cache, it doesn't matter how many times you reload the api request, it will still come from the preloaded middleware. My worry, is that even if we update the menu items, that the data will not update, as it is cache. By cache busting, i.e just add a query string to the api request, that will stop the request loading from middleware. |
const cacheData = cache[ method ][ path ]; | ||
|
||
// Unsetting the cache key ensures that the data is only preloaded a single time | ||
delete cache[ method ][ path ]; |
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 is a very big change to how preloading works. Are we saying here, after the first load, preload cache is removed. What about other problems of the code that may call the same rest api twice.
Cc @swissspidy
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 is not new/changed behavior? See
// Unsetting the cache key ensures that the data is only preloaded a single time |
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.
How is not a new behavior?
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.
That‘s from a few lines above. Sounds like it was not fully implemented there.
so this change here looks reasonable 👍
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.
That is a cache, it doesn't matter how many times you reload the api request, it will still come from the preloaded middleware.
This is not how it works at the moment.
We delete the cache data from the preloadedData object the first time we use the data, so nothing gets cached and we don't need to bust the caches.
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.
What about other problems of the code that may call the same rest api twice.
Then we need to fix those parts of the code that call the same REST API twice. We should call the same REST API twice only if it's absolutely needed.
IMO preloading cannot fix everything.
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.
Yeah, this here updates the cache logic for OPTIONS
requests to match the behavior for GET
requests. All reasonable to me.
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.
Annoying as it is we should consider pulling this out into a separate PR. That way it will be easier to notice (and rollback) if this introduces any unforeseen bugs.
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.
That way it will be easier to notice (and rollback) if this introduces any unforeseen bugs.
Other packages and scripts may use this package, this change of behaviour may break something. This needs to be a changelog and well documented.
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've moved this code into a separate PR.
#35527
We should be good to merge now.
lib/navigation-page.php
Outdated
* @param int $menu_id Menu ID. | ||
* @return string | ||
*/ | ||
function gutenberg_navigation_get_menu_endpoint( $menu_id ) { |
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.
Where are these functions used?
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.
Nowhere :) I was going to delete it. Good catch.
const cacheData = cache[ method ][ path ]; | ||
|
||
// Unsetting the cache key ensures that the data is only preloaded a single time | ||
delete cache[ method ][ path ]; |
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.
Should this be mentioned in the changelog? Bummer that #25550 didn't do that originally either
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.
Yes, it should. I will move this fix to a new PR.
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.
Approved with a nicpic 🎉
lib/navigation-page.php
Outdated
/** | ||
* This function returns an url for the /wp/v2/types endpoint | ||
* | ||
* @since 11.6.0 |
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.
It's going to be 11.8 now
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.
Fixed in 83a1edc
lib/navigation-page.php
Outdated
$menus_data = empty( $preload_data[ $menus_data_path ] ) ? | ||
array() | ||
: | ||
array( | ||
$menus_data_path => $preload_data[ $menus_data_path ], | ||
); |
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.
That's such a nit pick, but I find this ternary formatting pretty hard to read. Perhaps an if/else
would be more readable?
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 agree. It looks ugly.
I've fixed this, and it seems we don't need if/else
here, if
is enough.
* It uses data that is returned from the /__experimental/menus endpoint for requests | ||
* to the /__experimental/menu/<menuId> endpoint, because the data is the same. | ||
* This way, we can avoid making additional REST API requests. | ||
* This middleware can be removed if/when we implement caching at the wordpress/data level. |
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.
Would you create a ticket for caching at the wordpress/data level that so that we don't forget about it?
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.
Yes, #35601
if ( ! key ) { | ||
return next( options ); | ||
} | ||
|
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.
Is this check useful here?
if ( ! key ) { | |
return next( options ); | |
} |
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.
It's better safe than sorry, but I agree, we don't need that.
return id === menuId; | ||
} ); | ||
|
||
if ( 0 < menu.length ) { |
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.
Yoda conditions don't read well, e.g. here it's "if zero is less than menu length then." I know there are some guidelines to do that on PHP side to prevent accidental assignments like $a = 4
instead of $a === 5
, but these days linters are pretty good at catching that (and also it doesn't apply to <
).
if ( 0 < menu.length ) { | |
if ( menu.length ) { |
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.
Yoda conditions don't read well,
I see what you mean. I can agree it can be harder to read Yoda conditions for some people.
Although Yoda conditions have their advantages and it's not a crime to use them. IMO it's just a personal preference.
if ( value > 0 )
is better than just if ( value )
, because the former is a stricter comparison.
If value === -1
, the latter expression will return true
, which is not what it is supposed to do.
This can happen if we compare some unsigned numbers like bank account amounts.
I understand that length of the array cannot be lower than zero. But I prefer to write it like that everywhere because it will work correctly regardless of whether we compare unsigned or signed integers.
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.
Fixed (?) in 612d17b
My concern here is that we'll soon end up with |
lib/editor-settings.php
Outdated
* | ||
* @param Array $preload_data Array containing the preloaded data. | ||
*/ | ||
$preload_data = apply_filters( "{$editor_name}_preload_data", $preload_data ); |
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.
Is this a new filter? We should try to avoid using dynamic filter names.
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.
Hm, isn't this already filterable in block_editor_rest_api_preload
actually?
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.
Yes, this is a new filter.
We should try to avoid using dynamic filter names.
Good to know.
Fixed in 75519d9
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.
Why do we need this filter then? Why isn't it covered by block_editor_rest_api_preload
?
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.
@TimothyBJacobs
It isn't covered by block_editor_rest_api_preload
function because we need to modify the data that gets passed into the 'wp.apiFetch.use( wp.apiFetch.createPreloadingMiddleware( %s ) );'
.
My bad, I didn't notice you mentioned block_editor_rest_api_preload
the first time.
I can refactor this to use block_editor_rest_api_preload
instead, but:
- I will have to add a new filter into the body of the
block_editor_rest_api_preload
function because of the reason above. - We will break backward compatibility because we will have to get rid of the
$preload_paths = apply_filters( "{$editor_name}_preload_paths", $settings['preload_paths'] );
call.
The block_editor_rest_api_preload
function will replace all that code.
We need to refactor this, but I'm not sure about breaking BC. It seems that we don't use the filter above ({$editor_name}_preload_paths
).
What are your thoughts on this?
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.
What I'm having trouble understanding is why we can't use the block_editor_rest_api_preload_paths
filter in block_editor_rest_api_preload
. Why do we need another filter on a list of preloaded paths.
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 is not a filter on a list of preloaded paths.
This is the filter that allows modifying already preloaded data.
I've created createMenuPreloadingMiddleware
middleware, and it needs its own set of data, i.e., a list of all menus.
That filter allows modifying preloaded data so that list of all menus doesn't get passed into the createPreloadingMiddleware
.
Instead, we need to pass it into the createMenuPreloadingMiddleware
middleware and process it there.
We have to either push this PR with that additional filter OR remove that filter and the custom middleware (createMenuPreloadingMiddleware
).
In such a case, we won't be able to preload /__experimenta/menu/<menuId>
endpoints properly until we find a better solution.
What is your opinion on this?
Co-authored-by: Adam Zielinski <adam@adamziel.com>
2. Don't use variable filter name. Pass the ditor_name as the second parameter.
2. Remove redundant code.
Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
It's hard to tell why, but both requests are coming from this place: https://github.com/WordPress/gutenberg/blob/trunk/packages/core-data/src/resolvers.js#L105 |
* | ||
* @return {string} Normalized path. | ||
*/ | ||
export function getStablePath( path ) { |
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.
Looking at the way this function works, it's worth checking some of the utils in the url
package. It might be possible to make use of getQueryString
or getQueryArgs
from that package.
Some of this code might also be a good candidate for the url
package. I could see a normalizeQueryString
function extracted from this being a useful utility. There might also be overlap with buildQueryString
does that already order query string args?
Might be a good thing for a separate follow-up PR.
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.
Awesome! I've created a GH issue for this. #35799
* @param {Object} preloadedData | ||
* @return {Function} Preloading middleware. | ||
*/ | ||
export function createMenuPreloadingMiddleware( preloadedData ) { |
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.
Even though edit-navigation
isn't a public package, I'm uneasy about exporting this as a stable API because it's very specific to menus and the navigation editor.
It seems like a more general problem that when we preload a request that lists items the individual item can also be considered preloaded, regardless of the REST resource. It might be something that applies to posts, pages and other resources too. What do you think?
I'm not against merging this PR with a special case for menus right now, but in the long run I could see this as a candidate for removal if we come up with a more generalised preloading system.
Right now it might be worth exporting this as __unstableCreateMenuPreloadingMiddleware
so that it doesn't accidentally end up as a stable API that has to be supported indefinitely.
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'd wait until #15105 gets merged and remove createMenuPreloadingMIddleware
altogether.
When I started to work on this issue I wanted to implement something like that. I didn't know this PR already existed.
Thank you for the review.
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.
#15105 is an issue and seems unrelated. Did you mean another number?
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.
@adamziel The related discussion is here - #35402 (comment), the background is that we'd be able to preload the selected menu item if it were stored in the database instead of local storage, and I guess that removes the use case for this function? Though I still feel like we'll be in a situation where we need to load all the menus for the menu switcher.
@anton-vlasenko I wouldn't worry too much about waiting for #15105 / #19177 because it has been a very long-running issue, but it'd be good to help it along if we can.
If we can ship this PR by renaming the function I think that's fine 👍
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.
#15105 is an issue and seems unrelated. Did you mean another number?
It seems to be related. If it allows to store selectedMenuId
to the database and then fetch it from there, it will solve all the issues with preloading.
We have to tinker with createMenuPreloadingMiddleware
only because we don't know which menu is currently selected when preloading data.
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 for the feedback, @talldan.
I will surely rename that method. (UPD: fixed in 41ea183).
I'm waiting for @TimothyBJacobs 's feedback on this comment. #35402 (comment)
); | ||
|
||
if ( $first_menu_id ) { | ||
$preload_paths[] = gutenberg_navigation_get_menu_items_endpoint( $first_menu_id ); |
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 see there was already a resolved conversation about this, but I think it might be worth creating an issue to explore how the selected menu might be preloaded.
A thought I had is that we could pass the menu id as a query string http://localhost:8888/wp-admin/nav-menus.php?menuId=2
, but it'd require every link to the editor to have that query string 🤯 . Deep linking to a menu would be cool though. A query string wouldn't be perfect, even better would be a proper route like http://localhost:8888/wp-admin/menus/2
.
Or alternatively we could help with #15105, at which point the preference would be stored in the database rather than localStorage, and our server-side code would be able to use the value.
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.
Or alternatively we could help with #15105, at which point the preference would be stored in the database rather than localStorage, and our server-side code would be able to use the value.
That PR will be a game-changer if it works as you described. Then we could just use selectedMenuId
from the database to preload data and get rid of the createMenuPreloadingMiddleware altogether.
I'm not aware of how this is solved in different places.
I've tried to incorporate this into the |
We don't want it to become a stable API for now. See #35402 (comment)
The structure of this PR is confusing to me. I don't understand why we are introducing another filter in the block editor settings. I understand that this is filtering the preloaded data, instead of the set of paths. But I don't think that's the API that we should be exposing to 3rd party developers since it's a lot lower level. It seems like these permanent APIs are being added to solve temporary deficiencies in Have we considered preloading the different menus as separate requests? |
Yes, let's discuss it and maybe create follow-up tasks (given the age of this PR).
I'm not sure I understand.
What is your idea exactly? BTW, I've just realized we can get rid of that additional filter. // Of course this snippet has to be refined, it's just for example
$preload_data = array_reduce(
['/__experimental/menus'],
'rest_preload_api_request',
array()
); Would you please let me know if that would be an acceptable solution? |
Yeah, I don't have an issue with that
Yeah, not introducing that filter and implementing it as you suggest would be preferable to me. |
Yes, I created it several days ago. #35601 (feel free to add your notes to it).
Awesome! I will create a PR and an issue to refactor this. |
So, here is the PR that removes that filter. I'd appreciate your review. |
Description
When loading the Navigation screen, there are a lot of loading spinners due to API requests to /menus and /menu-items. We should preload these requests so that the screen appears faster.
The goal of this PR is to preload these requests.
Also, this PR fixes 2 small bugs in the
core-data
andapi-fetch
modules.Fixes #24642
How has this been tested?
wp-admin/admin.php?page=gutenberg-navigation
).Screenshots
Before:
After:
Types of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).