Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Fetch: Normalize keys of incoming preloaded data #18724

Merged
merged 2 commits into from Nov 26, 2019

Conversation

@aduth
Copy link
Member

aduth commented Nov 25, 2019

This pull request seeks to resolve an issue where preloaded data using createPreloadingMiddleware makes assumptions about the paths provided in the preloaded data, which if not met, can lead to unintentional cache misses when later API fetches attempt to reference the preloaded data.

Example:

createPreloadingMiddleware( {
    'wp/v2/demo?foo=bar&baz=quux': { body: { /* ... */ } }
} );

// ...

apiFetch( 'wp/v2/demo?baz=quux&foo=bar' );

Prior to these changes, the apiFetch call above would not use the preloaded data.

The preloading middleware internally normalizes paths to reference identical query parameters as the same, regardless of their original order. However, this normalization was not applied to incoming preloaded data. Thus, since the preloaded data above assigns keys in the non-normalized form, they would never be matched.

Testing Instructions:

Verify unit tests pass:

npm run test-unit packages/api-fetch/src/middlewares/test/preloading.js
Copy link
Contributor

youknowriad left a comment

LGTM but I wonder if this means we should push a Core patch to remove the normalization that is already happening on the backend (because I think there's something there)

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 26, 2019

At least, I do remember some normalization happening on the backend, maybe it's not the case anymore.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Nov 26, 2019

I don't think there's anything special happening on the backend.

I think it's just this hard-coded list:

https://github.com/WordPress/wordpress-develop/blob/29f0d74/src/wp-admin/edit-form-blocks.php#L42-L53

(There's no default filtering on block_editor_preload_paths and rest_preload_api_request doesn't modify the path in any way)

I'm actually curious though, looking at the default list, whether there are existing issues, since the normalization should be alphabetizing the order of the query parameters (thus, the preloading for /taxonomies and /users/me may not be matching?)

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Nov 26, 2019

I'm actually curious though, looking at the default list, whether there are existing issues, since the normalization should be alphabetizing the order of the query parameters (thus, the preloading for /taxonomies and /users/me may not be matching?)

It would appear this is the case. When loading the editor, I see a request for the /taxonomies and /users/me endpoints in the Chrome Network tab.

For one thing, I think we changed away from requesting per_page=-1, and instead use fetchAllMiddleware to request 100 taxonomies at a time. I expect we probably want to change this in core to just preload the first page of data.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Nov 26, 2019

For one thing, I think we changed away from requesting per_page=-1, and instead use fetchAllMiddleware to request 100 taxonomies at a time. I expect we probably want to change this in core to just preload the first page of data.

While this is true, the way it works is that a component will express their query as if they're fetching per_page: -1, and the middleware transforms the path to request 100 at a time. But for the purposes of preloading, it considers the path as per_page=-1.

It's still not matched because the preloaded parameter order is not normalized (fixed by this pull request).

Whether we should change the order of these middlewares so that it tries to use preloaded data for the segment of data actually being fetched (i.e. first page of per_page=100), it seems reasonable. It would be something to explore separately (in Gutenberg, not core).

} )
// [ [ 'a', '5' ], [ 'b, '1' ], [ 'c', '2' ] ]
.sort( function( a, b ) {
return a[ 0 ].localeCompare( b[ 0 ] );

This comment has been minimized.

Copy link
@nerrad

nerrad Nov 26, 2019

Contributor

TIL localeCompare. Nifty. How will this perform if the string values happen to be different locales though. Is that a concern here? I'm guessing no, but just thought I'd surface it.

This comment has been minimized.

Copy link
@aduth

aduth Nov 26, 2019

Author Member

TIL localeCompare. Nifty. How will this perform if the string values happen to be different locales though. Is that a concern here? I'm guessing no, but just thought I'd surface it.

Even if it differed, as long as it's consistent within a locale, it should work just as well.

Also noting, this wasn't added here. The pull request merely shuffled it around, and included some tests which were previously lacking.

@aduth aduth merged commit be671ea into master Nov 26, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@aduth aduth deleted the fix/api-fetch-normalize-preload-keys branch Nov 26, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.