API Fetch: Make preloaded OPTIONS requests use parse setting#28862
Merged
Conversation
Member
|
Another inconsistency that would be nice to resolve is that unlike |
Contributor
Author
|
If we're comfortable breaking backcompat here and are overall happy with the approach, I'd be totally happy to implement that @TimothyBJacobs |
|
Size Change: +6 B (0%) Total Size: 1.46 MB
ℹ️ View Unchanged
|
Contributor
Author
|
I have a version implemented locally that changes the response to use a true |
gziolo
approved these changes
Apr 14, 2021
gziolo
left a comment
Member
There was a problem hiding this comment.
It seems to be a good idea to align API for different methods of requests. Should we also include a note in the changelog of the package so people were aware of the new option added? Is there any higher level documentation that needs to be updated as well?
gziolo
reviewed
Apr 14, 2021
|
|
||
| ### Breaking changes | ||
|
|
||
| - `OPTIONS` requests which are handled by the preloading middleware are no longer resolved as unparsed responses unless you explicitly set `parse: false`, for consistency with other request methods. If you expect an unparsed response, add `{ parse: false }` to your request options to preserve the previous behavior. |
Member
There was a problem hiding this comment.
Does it impact any existing REST API calls in Gutenberg?
Contributor
Author
There was a problem hiding this comment.
It does not impact any existing calls in the Gutenberg codebase — I think the reason this wasn't noticed previously is that all the core requests already explicitly set { parse: false }… it just wasn't actually doing anything!
Member
There was a problem hiding this comment.
I can confirm it now that you mentioned it :)
gziolo
approved these changes
Apr 15, 2021
parse settingparse setting
Contributor
Author
|
@gziolo Thanks for merging — the performance tests were dragging on and I lost track of my open tab 😁 |
Member
|
Yes, it takes over 40 minutes, there is even an issue that prompts some action 😄 |
8 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
When preloading requests via the preloading middleware, the middleware changes the shape of the returned response for
GETrequests based on the value of theparseflag. However, it does not do the same forOPTIONSrequests, which are always returned in their unparsed form (with body and header properties).This behavior appears to originate with the work around verifying media upload capabilities in #4155.
As far as I can tell, Gutenberg/Core only makes OPTIONS requests with
parse: falseas the requests are looking for header data, so this change should not impact core code. (It already receives the response shape it expects to get, and will continue to do so.)How has this been tested?
Added unit tests.
Types of changes
This is technically a breaking change, if users implemented preloading expecting the incorrect response shape. I know there are larger discussions happening around issues with preloading, so it may make sense to punt this until those conversations are resolved.
Checklist: