Core Data: Stop sending duplicate requests in canUser resolver #43480
Conversation
|
Size Change: +690 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
Co-authored-by: George Mamadashvili <[email protected]>
|
The e2e failures are the same as in trunk. I think this is good to be merged as soon as anyone dares to review :D cc @noisysocks @jorgefilipecosta @jsnajdr @draganescu |
gziolo
changed the title
|
@adamziel, I just pushed a minor fix when checking resources without an ID + minor refactor and unit tests for this new case. Suppose anyone is looking for a way to test this fix manually. You can run the following snippet in the DevTools console and confirm that it only triggers one REST API request. |
|
Yay @Mamaduka, thank you! It looks good and makes sense to me. |
|
@Mamaduka I pushed one more bugfix and unit test. The site title block calls |
|
Nice catch, @adamziel. The third-party code might also use |
|
These may both be for another ticket. Making an When a request does need to be made, we should probably also explore a Core ticket around building specific Why does |
I think this is happening since #35527
I like the
My first thought was that it takes a resource to check more than just entity records permissions, but then I didn't find any instances of that. Good call about supporting other namespaces – would you please create a separate ticket for that? |
|
Actually, we already have gutenberg/packages/core-data/src/selectors.ts Lines 996 to 1009 in 1d778aa |
Right, when the Thanks @Mamaduka. I'm going to spin up a new ticket. That makes the DUX a bit nicer, but doesn't actually solve the issue of |
|
I like the idea of not throwing away Another thing we should probably look into is combining @TimothyBJacobs, how can we help with adding |
Sure – let's do that! |
I've created #43751.
Core has built-in support, but I'm now less sure it is a good idea. Going to comment in #43703. |
|
@Mamaduka here's two places to refactor to use the gutenberg/packages/core-data/src/resolvers.js Line 117 in ebf3791 gutenberg/packages/core-data/src/resolvers.js Line 181 in ebf3791 The key is using the apiFetch's wp.apiFetch({
path: '...',
method: 'HEAD',
parse: false
})
.then(response => response.headers.get('allow'))
.then(console.log)Would you like to start that PR? |
|
Do you suggest making another API request with I thought @TimothyBJacobs, the recommendation was to use the header from the I wonder if adding requests to these resolvers defeats the whole purpose of this optimization. In any case, I can start on this next week, and we'll see how the results. |
Not at all! I suggest making just one API request and make sure it uses |
|
I think that would break the current gutenberg/packages/api-fetch/src/middlewares/fetch-all-middleware.js Lines 77 to 81 in f26adef |
|
I think the |
| if ( isAlreadyResolving ) { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Shouldn't this be built into resolvers, instead of modifying a specific resolver? Or is that not possible?
There was a problem hiding this comment.
Ah, got it, you're matching similar resolvers.
What?
The following snippet issues three identical HTTP requests:
Each time, the
canUserresolver sends a simple OPTIONS request to the/wp/v2/pages/10.This PR caches and reuses the results retrieved the first time. It follows up on an observation @Mamaduka got after stabilizing the
useResourcePermissionshook:Testing Instructions
It's hard to test this in a browser so I added a test case to check how many API calls are issued. Confirm it makes sense and passes.
cc @Mamaduka @gziolo