Fix preloading middleware referencing stale data for OPTIONS requests #35527
Conversation
|
Size Change: +254 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
anton-vlasenko
added
the
[Type] Bug
anton-vlasenko
changed the title
anton-vlasenko
changed the title
| ); | ||
| const cacheData = cache[ method ][ path ]; | ||
|
|
||
| // Unsetting the cache key ensures that the data is only preloaded a single time |
There was a problem hiding this comment.
| // Unsetting the cache key ensures that the data is only preloaded a single time | |
| // Unsetting the cache key ensures that the preloaded data is only used a single time |
There was a problem hiding this comment.
Good catch!
Fixed in 8f41aaa
This text is not mine (I copied it from the code above), but I could have checked it.
There was a problem hiding this comment.
I was worried it would trigger additional OPTIONS requests during the initial load in cases when the editor calls apiFetch multiple times, but it didn't happen. The number of requests during loading is the same with and without this PR for all the editors I checked (site, post, navigation, widgets). LGTM 👍
Thank you for checking site, post, navigation and widgets editors. |
|
What if the same API call is requested several times on the initial page load? I see this PR mirrors the behavior with how It probably would make more sense to disable the preloading middleware completely after the page is completely rendered. The current approach won't help with stale data issues when the request wasn't called during the initial page load. |
That was my concern too, but it doesn't seem to be happening.
+1 |
|
I don't mind if we disable the preloading middleware when the page is loaded (in the scope of another PR of course).
If the same API call is issued several times on the initial page load, we probably need to refactor a code that makes a duplicate request. There must be very good reasons to request the same chunk of information twice. |
anton-vlasenko
force-pushed
the
fix/clear-cache-when-preloading-options-requests
branch
from
8f41aaa
to
ee11c3d
Compare
|
@kevin940726 Thank you for the review.
I've implemented a unit test (just FYI). I'm going to merge this PR then. |
|
We need to add this PR to the Gutenberg 11.8 release notes. |
anton-vlasenko
deleted the
fix/clear-cache-when-preloading-options-requests
branch
cc @priethor |
|
Hi @anton-vlasenko ! Given the not-too-technical orientation of the Gutenberg release posts, could you help draft a short explanation for the release post that helps put this change in context given the previous PR and convey its importance? 🙏 |
|
@priethor A few notes from me: It's important because we change how preloading works. It's very unlikely, but it could affect some 3rd party plugins. This is just a bug fix. |
I just came across this case when working on different PR. If Editor root calls It was a bit unexpected since I know While I understand the reasoning behind the #25550, how likely is it that prefetched |
@Mamaduka It will happen sometimes for sure but I wouldn't expect it to happen often. It would be great to have a consistent way of handling all such cases. |
|
Thanks, @adamziel. Currently, I can think of two options for avoiding extra API calls when checking different action permissions for the same resource.
Cc @swissspidy, @spacedmonkey. |
|
I don't think I am the right person to chime in here. Reverting this change sounds like the wrong choice though |
|
If I understand you correctly, calls options and calls to type definitions should always be caches right? As options / performation checks, so not changed in a session, this seems reasonable. Also types, like post types and taxonomines are very unlikely to change and also seem to reason to cache. I could get behind that change. |
|
The issue is more visible with the block that checks multiple permissions for a resource, like the Navigation block. Which uses |
Description
The goal of this PR is to fix stale middleware data for
OPTIONStype requests.Please check out that PR first to better understand the purpose of the fix.
Steps to reproduce the bug.
From middlewarestring in the console the first time you run this request. This is the response we are getting from the middleware and not the actual REST API response.Expected result:
You shouldn't see
From middlewarestring in the console when you execute the request the second time.That request should be made to the actual REST API endpoint. Middleware should only be used once.
Actual result:
You will see
From middlewareevery time you run the code mentioned in step 5.Steps to test this PR.
Steps to reproduce this bug.From middlewarestring only once (when you run the request for the first time).Screenshots
Types of changes
Bug fix.
Checklist:
*.native.jsfiles for terms that need renaming or removal).