Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
7250125
Add preloading for /__experimental/menus endpoint.
anton-vlasenko Oct 1, 2021
8c2df90
Add a new filter to allow to modify the data that has already been pr…
anton-vlasenko Oct 1, 2021
eb69f2d
Commit WIP.
anton-vlasenko Oct 1, 2021
482eb34
Preload more endpoints.
anton-vlasenko Oct 4, 2021
d06b9e0
Fix preload paths.
anton-vlasenko Oct 4, 2021
b42deac
Don't add slash if the key is empty.
anton-vlasenko Oct 4, 2021
55a460d
Preload /wp/v2/types URLs.
anton-vlasenko Oct 4, 2021
282e7e0
Commit WIP.
anton-vlasenko Oct 5, 2021
8b55372
1. Fix the test.
anton-vlasenko Oct 6, 2021
630dc7a
Fix indents.
anton-vlasenko Oct 6, 2021
24daf14
Add PHPDOC blocks to the public functions introduced in this PR.
anton-vlasenko Oct 6, 2021
7a85496
Add @since tags.
anton-vlasenko Oct 6, 2021
254c788
OPTIONS type requests should be cached the same way as GET type requets.
anton-vlasenko Oct 6, 2021
c8b4eed
Fix the code style.
anton-vlasenko Oct 6, 2021
14e4d0f
Use WP function build_query instead of http_build_query.
anton-vlasenko Oct 6, 2021
6f526a2
Fix code style.
anton-vlasenko Oct 6, 2021
94b412e
Format the code.
anton-vlasenko Oct 7, 2021
f071d8d
Add a new filter to modify the preloaded data.
anton-vlasenko Oct 8, 2021
b206204
Wire up createMenuPreloadingMiddleware so that we can preload menus d…
anton-vlasenko Oct 8, 2021
61c150b
Fix PHPDOC blocks.
anton-vlasenko Oct 8, 2021
436e7c7
Add PHPDOC block.
anton-vlasenko Oct 8, 2021
535f4e3
Implement menu preloading middleware.
anton-vlasenko Oct 11, 2021
a34a2ee
Remove the gutenberg_navigation_get_menu_endpoint function. It's not …
anton-vlasenko Oct 11, 2021
c16d3ee
We should only load data once.
anton-vlasenko Oct 11, 2021
6ccc620
Add PHPDOC block.
anton-vlasenko Oct 11, 2021
24ba0b5
Fix the unit test.
anton-vlasenko Oct 11, 2021
87f5c98
Improve test code coverage. Add a new unit test for test_gutenberg_na…
anton-vlasenko Oct 11, 2021
c58002e
Update lib/editor-settings.php
anton-vlasenko Oct 11, 2021
f37e481
Fix code style.
anton-vlasenko Oct 11, 2021
368367b
Fix code style.
anton-vlasenko Oct 11, 2021
79a7e3a
Fix code style.
anton-vlasenko Oct 11, 2021
fcfdc8c
Fix bug in createMenuPreloadingMiddleware.
anton-vlasenko Oct 11, 2021
4d5878b
Revert this change as it needs to be moved into a separate PR.
anton-vlasenko Oct 11, 2021
6fc54fb
Add public access modifier to the functions.
anton-vlasenko Oct 11, 2021
83a1edc
Fix version name.
anton-vlasenko Oct 12, 2021
a62c907
Update packages/edit-navigation/src/utils/index.js
anton-vlasenko Oct 12, 2021
612d17b
Remove the yoda condition as it can be harder to read it for some peo…
anton-vlasenko Oct 12, 2021
75519d9
1. Format the code.
anton-vlasenko Oct 12, 2021
d9f51e2
1. Update PHPDOC block.
anton-vlasenko Oct 12, 2021
eedbff5
Update lib/navigation-page.php
anton-vlasenko Oct 13, 2021
78b501e
Fix the comment.
anton-vlasenko Oct 13, 2021
41ea183
Make the middleware unstable.
anton-vlasenko Oct 15, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions lib/editor-settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,18 @@ function gutenberg_initialize_editor( $editor_name, $editor_script_handle, $sett
'rest_preload_api_request',
array()
);

/**
* Filters the array of data that has been preloaded.
*
* The dynamic portion of the hook name, `$editor_name`, refers to the type of block editor.
*
* @param array $preload_data Array containing the preloaded data.
* @param string $editor_name Current editor name.
* @param array Array containing the filtered preloaded data.
*/
$preload_data = apply_filters( 'block_editor_preload_data', $preload_data, $editor_name );

wp_add_inline_script(
'wp-api-fetch',
sprintf(
Expand Down
105 changes: 102 additions & 3 deletions lib/navigation-page.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,84 @@ class="edit-navigation"
}

/**
* Initialize the Gutenberg Navigation page.
* This function returns an url for the /__experimental/menus endpoint
*
* @since 7.8.0
* @since 11.8.0
*
* @param int $results_per_page Results per page.
* @return string
*/
function gutenberg_navigation_get_menus_endpoint( $results_per_page = 100 ) {
return '/__experimental/menus?' . build_query(
array(
'per_page' => $results_per_page,
'context' => 'edit',
'_locale' => 'user',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Contributor Author

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.

Copy link
Contributor

@adamziel adamziel Oct 12, 2021

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. 😒

)
);
}

/**
* This function returns an url for the /__experimental/menu-items endpoint
*
* @since 11.8.0
*
* @param int $menu_id Menu ID.
* @param int $results_per_page Results per page.
* @return string
*/
function gutenberg_navigation_get_menu_items_endpoint( $menu_id, $results_per_page = 100 ) {
return '/__experimental/menu-items?' . build_query(
array(
'context' => 'edit',
'menus' => $menu_id,
'per_page' => $results_per_page,
'_locale' => 'user',
)
);
}

/**
* This function returns an url for the /wp/v2/types endpoint
*
* @since 11.8.0
*
* @return string
*/
function gutenberg_navigation_get_types_endpoint() {
return '/wp/v2/types?' . build_query(
array(
'context' => 'edit',
)
);
}

/**
* Initialize the Gutenberg Navigation page.
*
* @param string $hook Page.
* @since 7.8.0
*/
function gutenberg_navigation_init( $hook ) {
if ( 'gutenberg_page_gutenberg-navigation' !== $hook ) {
return;
return;
}

$menus = wp_get_nav_menus();
$first_menu_id = ! empty( $menus ) ? $menus[0]->term_id : null;

$preload_paths = array(
'/__experimental/menu-locations',
array( '/wp/v2/pages', 'OPTIONS' ),
array( '/wp/v2/posts', 'OPTIONS' ),
gutenberg_navigation_get_menus_endpoint(),
gutenberg_navigation_get_types_endpoint(),
);

if ( $first_menu_id ) {
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
$preload_paths[] = gutenberg_navigation_get_menu_items_endpoint( $first_menu_id );
Copy link
Contributor

@talldan talldan Oct 14, 2021

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.

Copy link
Contributor Author

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.

}

$settings = array_merge(
gutenberg_get_default_block_editor_settings(),
array(
Expand Down Expand Up @@ -82,3 +143,41 @@ function gutenberg_navigation_editor_load_block_editor_scripts_and_styles( $is_b
}

add_filter( 'should_load_block_editor_scripts_and_styles', 'gutenberg_navigation_editor_load_block_editor_scripts_and_styles' );

/**
* This function removes menu-related data from the "common" preloading middleware and calls
* createMenuPreloadingMiddleware middleware because we need to use custom preloading logic for menus.
*
* @param Array $preload_data Array containing the preloaded data.
* @param string $context Current editor name.
* @return array Filtered preload data.
*/
function gutenberg_navigation_editor_preload_menus( $preload_data, $context ) {
if ( 'navigation_editor' !== $context ) {
return $preload_data;
}

$menus_data_path = gutenberg_navigation_get_menus_endpoint();
$menus_data = array();
if ( ! empty( $preload_data[ $menus_data_path ] ) ) {
$menus_data = array( $menus_data_path => $preload_data[ $menus_data_path ] );
}

if ( ! $menus_data ) {
return $preload_data;
}

wp_add_inline_script(
'wp-edit-navigation',
sprintf(
'wp.apiFetch.use( wp.editNavigation.__unstableCreateMenuPreloadingMiddleware( %s ) );',
wp_json_encode( $menus_data )
),
'after'
);

unset( $preload_data[ $menus_data_path ] );
return $preload_data;
}

add_filter( 'block_editor_preload_data', 'gutenberg_navigation_editor_preload_menus', 10, 2 );
2 changes: 1 addition & 1 deletion packages/core-data/src/resolvers.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export const getEntityRecord = ( kind, name, key = '', query ) => async ( {
// for how the request is made to the REST API.

// eslint-disable-next-line @wordpress/no-unused-vars-before-return
const path = addQueryArgs( entity.baseURL + '/' + key, {
const path = addQueryArgs( entity.baseURL + ( key ? '/' + key : '' ), {
...entity.baseURLParams,
...query,
} );
Expand Down
2 changes: 2 additions & 0 deletions packages/edit-navigation/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,5 @@ export function initialize( id, settings ) {
document.getElementById( id )
);
}

export { createMenuPreloadingMiddleware as __unstableCreateMenuPreloadingMiddleware } from './utils';
122 changes: 122 additions & 0 deletions packages/edit-navigation/src/utils/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we document this function with

  1. Why it exists.
  2. High level overview of what it is doing.

🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 24db010

* The purpose of this function is to create a middleware that is responsible for preloading menu-related data.
* 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/core-data level.
*
* @param {Object} preloadedData
* @return {Function} Preloading middleware.
*/
export function createMenuPreloadingMiddleware( preloadedData ) {
Copy link
Contributor

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.

Copy link
Contributor Author

@anton-vlasenko anton-vlasenko Oct 14, 2021

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.

Copy link
Contributor

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?

Copy link
Contributor

@talldan talldan Oct 15, 2021

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamziel

#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.

Copy link
Contributor Author

@anton-vlasenko anton-vlasenko Oct 15, 2021

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)

const cache = Object.keys( preloadedData ).reduce( ( result, path ) => {
result[ getStablePath( path ) ] = preloadedData[ path ];
return result;
}, /** @type {Record<string, any>} */ ( {} ) );

let menusDataLoaded = false;
let menuDataLoaded = false;

return ( options, next ) => {
const { parse = true } = options;
if ( 'string' !== typeof options.path ) {
return next( options );
}

const method = options.method || 'GET';
if ( 'GET' !== method ) {
return next( options );
}

const path = getStablePath( options.path );
if ( ! menusDataLoaded && cache[ path ] ) {
menusDataLoaded = true;
return sendSuccessResponse( cache[ path ], parse );
}

if ( menuDataLoaded ) {
return next( options );
}

const matches = path.match(
/^\/__experimental\/menus\/(\d+)\?context=edit$/
);
if ( ! matches ) {
return next( options );
}

const key = Object.keys( cache )?.[ 0 ];
const menuData = cache[ key ]?.body;
if ( ! menuData ) {
return next( options );
}

const menuId = parseInt( matches[ 1 ] );
const menu = menuData.filter( ( { id } ) => id === menuId );

if ( menu.length > 0 ) {
menuDataLoaded = true;
// We don't have headers because we "emulate" this request
return sendSuccessResponse(
{ body: menu[ 0 ], headers: {} },
parse
);
}

return next( options );
};
}

/**
* This is a helper function that sends a success response.
*
* @param {Object} responseData An object with the menu data
* @param {boolean} parse A boolean that controls whether to send a response or just the response data
* @return {Object} Resolved promise
*/
function sendSuccessResponse( responseData, parse ) {
return Promise.resolve(
parse
? responseData.body
: new window.Response( JSON.stringify( responseData.body ), {
status: 200,
statusText: 'OK',
headers: responseData.headers,
} )
);
}

/**
* Given a path, returns a normalized path where equal query parameter values
* will be treated as identical, regardless of order they appear in the original
* text.
*
* @param {string} path Original path.
*
* @return {string} Normalized path.
*/
export function getStablePath( path ) {
Copy link
Contributor

@talldan talldan Oct 14, 2021

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.

Copy link
Contributor Author

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

const splitted = path.split( '?' );
const query = splitted[ 1 ];
const base = splitted[ 0 ];
if ( ! query ) {
return base;
}

// 'b=1&c=2&a=5'
return (
base +
'?' +
query
// [ 'b=1', 'c=2', 'a=5' ]
.split( '&' )
// [ [ 'b, '1' ], [ 'c', '2' ], [ 'a', '5' ] ]
.map( ( entry ) => entry.split( '=' ) )
// [ [ 'a', '5' ], [ 'b, '1' ], [ 'c', '2' ] ]
.sort( ( a, b ) => a[ 0 ].localeCompare( b[ 0 ] ) )
// [ 'a=5', 'b=1', 'c=2' ]
.map( ( pair ) => pair.join( '=' ) )
// 'a=5&b=1&c=2'
.join( '&' )
);
}
85 changes: 85 additions & 0 deletions phpunit/navigation-page-test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
<?php
/**
* This class is supposed to test the functionality of the navigation-page.php
*
* @package Gutenberg
*/

class WP_Navigation_Page_Test extends WP_UnitTestCase {
/**
* @var WP_Navigation_Page_Test_Callback
*/
private $callback;

public function setUp() {
parent::setUp();
$this->callback = $this->createMock( WP_Navigation_Page_Test_Callback::class );
add_filter( 'navigation_editor_preload_paths', array( $this->callback, 'preload_paths_callback' ) );
add_filter( 'wp_get_nav_menus', array( $this->callback, 'wp_nav_menus_callback' ) );
}

public function tearDown() {
parent::tearDown();
remove_filter( 'navigation_editor_preload_paths', array( $this->callback, 'preload_paths_callback' ) );
remove_filter( 'wp_get_nav_menus', array( $this->callback, 'wp_nav_menus_callback' ) );
}

public function test_gutenberg_navigation_init_function_generates_correct_preload_paths() {
$menu_id = mt_rand( 1, 1000 );
$expected_preload_paths = array(
'/__experimental/menu-locations',
array(
'/wp/v2/pages',
'OPTIONS',
),
array(
'/wp/v2/posts',
'OPTIONS',
),
'/__experimental/menus?per_page=100&context=edit&_locale=user',
'/wp/v2/types?context=edit',
"/__experimental/menu-items?context=edit&menus={$menu_id}&per_page=100&_locale=user",
);

$this->callback->expects( $this->once() )
->method( 'preload_paths_callback' )
->with( $expected_preload_paths )
->willReturn( array() );

$menu = new stdClass();
$menu->term_id = $menu_id;
$this->callback->expects( $this->once() )
->method( 'wp_nav_menus_callback' )
->with( array() )
->willReturn( array( new WP_Term( $menu ) ) );

set_current_screen( 'gutenberg_page_gutenberg-navigation' );
gutenberg_navigation_init( 'gutenberg_page_gutenberg-navigation' );
}

public function test_gutenberg_navigation_editor_preload_menus_function_returns_correct_data() {
$menus_endpoint = gutenberg_navigation_get_menus_endpoint();
$preload_data = array(
'/__experimental/menu-locations' => array( 'some menu locations' ),
'OPTIONS' => array(
array( 'some options requests' ),
),
$menus_endpoint => ( 'some menus' ),
);

$result = gutenberg_navigation_editor_preload_menus( $preload_data, 'navigation_editor' );
$this->assertArrayHasKey( '/__experimental/menu-locations', $result );
$this->assertArrayHasKey( 'OPTIONS', $result );
$this->assertArrayNotHasKey( $menus_endpoint, $result );
}
}


/**
* This is a utility test class for creating mocks of the callback functions
*/
class WP_Navigation_Page_Test_Callback {

public function preload_paths_callback() {}
public function wp_nav_menus_callback() {}
}