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

Support recursively fetching paginated collections within apiFetch module #10845

Closed
wants to merge 4 commits into from

Conversation

kadamwhite
Copy link
Contributor

@kadamwhite kadamwhite commented Oct 21, 2018

For #6694

Description

Alter apiFetch module to detect unbounded (per_page=-1) API requests to native (wp/v2) WordPress endpoints, then recursively fetch and merge each page of data into a single array response using the maximum allowed per_page value.

This PR builds upon but supersedes #10762, as I have concluded that building this functionality directly into the core logic of apiFetch is a more reliable solution than using a middleware method. This is for several reasons:

  • Error detection and parsing happens within apiFetch, before the request is run through middleware, and trying to detect and apply the same logic from a middleware function was not possible for me despite several hours of attempts.
  • apiFetch has the most direct access to the relevant headers, and adding the logic here lets us avoid changing the parsing behavior specified by the consuming function; this will avoid potentially impacting other middleware methods.

While @youknowriad suggested this logic belongs in the data resolvers and not apiFetch or middleware, I consider building it in at this level to be the most pragmatic, simplest & most maintainable solution, and I propose we pursue changing the invocation mechanism from detecting per_page=-1 to a new flag argument to mitigate the concerns around when this logic should be applied instead of moving this logic to the resolvers.

Room for improvement which I do not consider critical for 5.0, but which could be addressed in future versions of Gutenberg:

  • We could use the parse-link-header npm module instead of my single-purpose implementation. I did not add it at this time partly because it would have added additional weight to the bundle, and partly because I could not successfully determine how to add a module to a Gutenberg package on the first try, and determined that implementing this logic was a more valuable use of time than learning how to manage repo dependencies :)
  • While the wp/v2 detection will avoid conflicts with third-party code which does support -1, it was suggested in Implement fetchAllMiddleware to handle per_page=-1 through pagination #10762 that we should consider using a new flag on the apiFetch options argument to trigger pagination rather than relying on detecting the per_page=-1 argument. [Edit: now supported, following discovery of a logical error which required a property of this type]
  • While no core endpoint of which I am aware currently both accepts a per_page argument and returns an object response (as opposed to an array), we could harden this against such endpoints by implementing some type of object response merging behavior. For 5.0 that line of thought raises more questions than it answers, so I have handled only the simple (and immediately useful) array handling case.

How has this been tested?

Tested by provisioning a local environment with 450 categories, and 250 users, etc. Requests asking for -1 values were correctly observed to return from the apiFetch module as merged arrays containing the data of every relevant page.

Types of changes

  • Alters apiFetch to detect per_page=-1 requests against first-part WordPress REST API endpoints, and contextually paginates through each available page of the collection.
    • Endpoint requests to wp/v2 namespaced endpoints which do not specify per_page=-1 are returned as normal, with no special processing.
    • Endpoints outside wp/v2 are returned as normal, with no special processing, as a forward compatibility guard to avoid compromising any plugin which does expose per_page=-1.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • (N/A: no interface component) My code follows the accessibility standards.
  • My code has proper inline documentation.

Alter `apiFetch` module to detect unbounded (`per_page=-1`) API requests
to native (`wp/v2`) WordPress endpoints, then recursively fetch and merge
each page of data into a single response array using the maximum allowed
`per_page` value.
An improperly written helper method was causing all requests to page,
hiding a logical error whereby subsequent pages of data would not be
properly interpreted in a way that continues pagination traversal.

The flag `allPages` will trigger paging behavior regardless of per_page.
@kadamwhite
Copy link
Contributor Author

we should consider using a new flag on the apiFetch options argument to trigger pagination

This is now supported, due to an overlooked logical error where we require a way to identify that paging recursion should continue for subsequent collection pages which do not contain per_page=-1 in their URL. I have chosen the name allPages for this argument as an expedient, but have no strong preference.

Usage:

// Recurse through available pages regardless of the per_page param's value:
apiFetch( {
    ...otherOptions,
    allPages: true,
} );

@pento pento added [Priority] High Used to indicate top priority items that need quick attention REST API Interaction Related to REST API [Package] API fetch /packages/api-fetch labels Oct 21, 2018
@pento pento added this to the 4.1 - UI freeze milestone Oct 21, 2018
@pento
Copy link
Member

pento commented Oct 21, 2018

This needs to go into the 4.1 release, for compatibility with WordPress 5.0 beta 1.

@danielbachhuber
Copy link
Member

Tested by provisioning a local environment with 450 categories, and 250 users, etc. Requests asking for -1 values were correctly observed to return from the apiFetch module as merged arrays containing the data of every relevant page.

How does the UI reflect this change? Does the UI begin to display when there's some data, or does it wait to display until all of the data is present?

@kadamwhite
Copy link
Contributor Author

@danielbachhuber Neither this PR nor #10762 alters the UI -- I don't have the number of the ticket to add a loading spinner handy, but the behavior with only this PR is that the delay before the categories list appears lengthens linearly the more categories you have.

@danielbachhuber
Copy link
Member

but the behavior with only this PR is that the delay before the categories list appears lengthens linearly the more categories you have.

Ok, good. This is the expected behavior: don't display UI until the data is fully resolved.

@danielbachhuber danielbachhuber removed this from the 4.1 - UI freeze milestone Oct 21, 2018
@danielbachhuber
Copy link
Member

Closing in favor of #10762

@danielbachhuber danielbachhuber deleted the add/apifetch-pagination-handling branch October 21, 2018 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] API fetch /packages/api-fetch [Priority] High Used to indicate top priority items that need quick attention REST API Interaction Related to REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants