Patterns: receive intermediate responses while unbound request is resolving #66713
+61
−16
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
|
Size Change: +181 B (+0.01%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
ellatrix
commented
Nov 4, 2024
| <BlockPatternPlaceholder key={ pattern.name } /> | ||
| ); | ||
| } ) } | ||
| { /* To do: async render within BlockPattern. Hasn't this already been done? */ } |
There was a problem hiding this comment.
Yes, this was implemented differently in #60425, but only for the site editor. I'll fix this in a separate PR. We should move Async to components and use it here as well.
The list change is causing all these patterns to re-render and is causing flickering.
I actually also don't see a performance regression in the PR.
There was a problem hiding this comment.
Made #66744 to handle it before or after this merge.
There was a problem hiding this comment.
Rebased on top of that now.
youknowriad
reviewed
Nov 4, 2024
| @@ -279,6 +289,44 @@ export const getEntityRecords = | |||
| response.headers.get( 'X-WP-TotalPages' ) | |||
| ), | |||
| }; | |||
| } else if ( | |||
| query.per_page === -1 && | |||
| query[ RECEIVE_INTERMEDIATE_RESULTS ] === true | |||
There was a problem hiding this comment.
I actually can see this as being useful as a public API. That said, I wonder about the impact on the "resolution state" and things like that.
The other thing I'm wondering, is that basically this means that the "pagination" logic for "-1" has moved within the resolver, so there's a potential to actually remove the code that is responsible for this in apiFetch and just reuse the same code here. The only different being that the action to "fill" the state won't be call until the end if RECEIVE_INTERMEDIATE_RESULTS is false.
There was a problem hiding this comment.
We could. I also saw that the resolver is anyway already dependent on knowledge of WP Total Pages etc., seen in trunk already 20 lines below. Do you think we should do it here or try separately?
Regarding the resolution state, I guess that's one thing to talk about before making this public. I think it's fine to resolve only after all requests are completed. It does not prevent components from selecting the unresolved state.
There was a problem hiding this comment.
Do you think we should do it here or try separately?
I'd say separately.
There was a problem hiding this comment.
I don't mind separate but I feel like we can follow-up on this rather than leave it as nice to improve later.
There was a problem hiding this comment.
@youknowriad Well, can we remove it from api-fetch though? We'd break back compat? Not all -1 requests necessarily go through this resolver.
There was a problem hiding this comment.
Good point :) maybe we can deprecate, WDYT?
There was a problem hiding this comment.
We could I guess. I'll look into it tomorrow. In the meantime, I'll merge this PR so the performance problem is addressed.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
mcsf
approved these changes
Nov 5, 2024
ellatrix
force-pushed
the
try/patterns-receive-while-resolving
branch
from
8fc0eb6
to
15fb801
Compare
|
I'd like to invite @kevin940726 @ndiego and @colorful-tones to review this PR and help determine whether it should be included in WP 6.7 release. My current feeling is that many sites will have lots of Patterns and having a frozen inserter is a very poor experience. However, my understanding is that this issue existed in WP 6.6 and thus wasn't introduced as part of this release. As a result I'm in two minds about including it. |
|
Same here, I can see both sides. On the bright side it's a fairly isolated change, only affects one selector call and request. |
|
Given the time, I think we should leave it out of the release because it has been working like that kind of forever. |
I agree. This is a fantastic improvement, but since it's not directly related to 6.7, it really should not be included at this point in the release cycle. We could always consider for 6.7.1. |
|
Ok let's go for 6.7.1 👍 Thanks for your input everyone. |
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?
Fixes #65569.
A temporary solution as removing the unbound -1 request will require a bigger refactor.
Why?
This is urgent because the are WP installations with thousands of patterns, and the inserter will not load anything unless all user patterns have been fetched from the server.
How?
Currently
getEntityRecordswithper_pageset to-1triggers a special case in the resolver that triggers a special version ofapiFetchthat waits for all responses it needs until resolving. The solution for this particular case would be to callapiFetchpage by page, and after each call receive the entities so they can be used in the UI.We may need to introduce a new, private query parameter for this new behaviour so it doesn't affect any other existing queries.
Testing Instructions
wp eval 'for ($i = 1; $i <= 3000; $i++) { wp_insert_post(["post_title" => "Reusable Block " . $i, "post_content" => "<!-- wp:paragraph --><p>This is block " . $i . "</p><!-- /wp:paragraph -->", "post_status" => "publish", "post_type" => "wp_block"]); }'Testing Instructions for Keyboard
Screenshots or screencast