Update: Request only needed fields on parent page selector #18586
Conversation
| @@ -54,6 +54,7 @@ const applyWithSelect = withSelect( ( select ) => { | |||
| parent_exclude: postId, | |||
| orderby: 'menu_order', | |||
| order: 'asc', | |||
| _fields: 'id,parent,title', | |||
There was a problem hiding this comment.
We should not use getEntityRecords with something that change the result format like _fields / context because otherwise it will potentially break other parts of the UI relying on the same entities. Entities should have a consistent format in the store.
There was a problem hiding this comment.
I was thinking that I was missing something, and this was it. In this case, should we not use getEntityRecords, or would it be possible to create an "entity" specific for this? Thank you for the review!
There was a problem hiding this comment.
Maybe it's fine to have a specific apiFetch call here? cc @aduth
There was a problem hiding this comment.
I've been thinking about this for another use-case, to try to limit the fields to omit post.content.rendered for the edited post, as a performance optimization.
The tricky part is the same as what you're experiencing here, in that the data can't be reused.
But I was under the impression it wouldn't necessarily break other usage, it would just treat it as a distinct query, where a separate request would would be made, and no reuse between known fields.
Ideally, I would like to explore some solution which tries to make the most reuse out of the known fields of a request, and only requests the sum difference from what's known. (See #15114)
In the meantime, I think this change is only worthwhile if we aren't using other fields elsewhere for the parent post. If we aren't, I would first check to confirm two _fields requests can coexist, albeit fetched separately. If we are, then it may not be a worthwhile optimization, or we should artificially align the _fields for what's needed across the two, so we can still take advantage of fields while only issuing one request.
There was a problem hiding this comment.
But I was under the impression it wouldn't necessarily break other usage, it would just treat it as a distinct query
Not if you use getEntityRecord to get a single entity, it will trigger the fetch request but will also return the existing cached value I think.
There was a problem hiding this comment.
Hm, well, that should definitely be fixed. Whether we decide to do that in parallel with introducing a specific apiRequest interim solution here or as a blocker (depending on the urgency of this issue), I don't have a strong preference.
|
This may be slightly off-topic but I want to add that ordering by menu_order causes undefined behavior in mysql if there is more than 100 pages in wordpress and they have the same menu_order. |
|
I'm closing this PR as currently, it has some problems. The PR should be unblocked after the _fields related changes @aduth is working on. |
youknowriad
deleted the
update/request-only-needed-fields-on-parent-page-selector
branch
Description
This PR comes as a result of the support topic https://wordpress.org/support/topic/slow-parent-page-attribute-for-websites-with-many-pages/.
Currently, we request a full page to compute the parent page UI. This may contain lots of information and make the request very slow to load.
This PR makes sure we request only the required fields.
How has this been tested?
I verified that the parent page selector still works as expected.
I verified the on browser developer tools that the only fiends requested were id, parent, and title.