From e24f311d5ca9dae93e2db2c94f0eb928881602ce Mon Sep 17 00:00:00 2001 From: "K. Adam White" Date: Thu, 18 Oct 2018 17:06:18 -0400 Subject: [PATCH 01/21] Implement fetchAllMiddleware to handle per_page=-1 through pagination Introduce a new middleware function to iterate through all available pages for a large collection, replacing any encountered `per_page=-1` requests with a series of sequential requests which are assembled into a final merged array of all available results. --- packages/api-fetch/src/index.js | 80 ++++++++++++++++++++++++++++----- 1 file changed, 70 insertions(+), 10 deletions(-) diff --git a/packages/api-fetch/src/index.js b/packages/api-fetch/src/index.js index a2ebeeff37a9a9..1e9563969b6c7a 100644 --- a/packages/api-fetch/src/index.js +++ b/packages/api-fetch/src/index.js @@ -26,9 +26,17 @@ function checkCloudflareError( error ) { } } +function parseResponse( response, parse = true ) { + if ( parse ) { + return response.json ? response.json() : Promise.reject( response ); + } + + return response; +} + function apiFetch( options ) { const raw = ( nextOptions ) => { - const { url, path, body, data, parse = true, ...remainingOptions } = nextOptions; + const { url, path, body, data, parse, ...remainingOptions } = nextOptions; const headers = remainingOptions.headers || {}; if ( ! headers[ 'Content-Type' ] && data ) { headers[ 'Content-Type' ] = 'application/json'; @@ -51,17 +59,9 @@ function apiFetch( options ) { throw response; }; - const parseResponse = ( response ) => { - if ( parse ) { - return response.json ? response.json() : Promise.reject( response ); - } - - return response; - }; - return responsePromise .then( checkStatus ) - .then( parseResponse ) + .then( ( response ) => parseResponse( response, parse ) ) .catch( ( response ) => { if ( ! parse ) { throw response; @@ -119,6 +119,66 @@ function apiFetch( options ) { apiFetch.use = registerMiddleware; +const capPerPageQuery = ( options ) => ( { + ...options, + // Swap back to requesting the max of 100 items per page. + // TODO: This feels brittle. Is there a better way to manage request parameters? + path: options.path && `${ options.path.replace( /(\&?per_page)=-1/, '$1=100' ) }&page=1`, + url: options.url && `${ options.url.replace( /(\&?per_page)=-1/, '$1=100' ) }&page=1`, +} ); + +const setRequestedPage = ( options, page ) => ( { + ...options, + path: options.path && options.path.replace( '&page=1', `&page=${ page }` ), + url: options.url && options.url.replace( '&page=1', `&page=${ page }` ), +} ); + +// The REST API enforces an upper limit of 100 on the per_page option. To handle +// large collections, apiFetch consumers can pass `per_page=-1` and this middleware +// function will recursively assemble a full response by paging over all available +// pages of API data. +const fetchAllMiddleware = async ( options, next ) => { + try { + if ( options.url && options.url.indexOf( 'per_page=-1' ) < 0 ) { + return next( options ); + } + + const pageOptions = capPerPageQuery( options ); + const pageOneResults = await next( { + ...pageOptions, + // Ensure headers are returned for page 1. + parse: false, + } ); + const pageOneParsed = await parseResponse( pageOneResults, options.parse ); + + const totalPages = pageOneResults.headers && pageOneResults.headers.get( 'X-WP-TotalPages' ); + + // Build an array of options objects for each remaining page. + const remainingPageRequests = Array.from( { + length: totalPages - 1, + } ).map( ( _, idx ) => { + // Specify the page to request. First additional request is index 0 but page 2, etc. + // Build these URLs based on pageOptions to avoid repeating the per_page adjustment. + return setRequestedPage( pageOptions, idx + 2 ); + } ); + + return remainingPageRequests.reduce( + // Request each remaining page in sequence, and return a merged array. + async ( previousPageRequest, nextPageOptions ) => { + const resultsCollection = await previousPageRequest; + const nextPage = await apiFetch( nextPageOptions ); + return resultsCollection.concat( nextPage ); + }, + // Ensure that the first page is parsed properly, as specified in the original options. + [].concat( pageOneParsed ) + ); + } catch ( e ) { + return Promise.reject( e ); + } +}; + +apiFetch.use( fetchAllMiddleware ); + apiFetch.createNonceMiddleware = createNonceMiddleware; apiFetch.createPreloadingMiddleware = createPreloadingMiddleware; apiFetch.createRootURLMiddleware = createRootURLMiddleware; From a3f3517af7947f2cb9e5c00455fbad7229508384 Mon Sep 17 00:00:00 2001 From: "K. Adam White" Date: Thu, 18 Oct 2018 17:48:42 -0400 Subject: [PATCH 02/21] Move fetchAllMiddleware to its own module --- packages/api-fetch/src/index.js | 62 +--------------- .../src/middlewares/fetch-all-middleware.js | 71 +++++++++++++++++++ 2 files changed, 73 insertions(+), 60 deletions(-) create mode 100644 packages/api-fetch/src/middlewares/fetch-all-middleware.js diff --git a/packages/api-fetch/src/index.js b/packages/api-fetch/src/index.js index 1e9563969b6c7a..aa73db04820858 100644 --- a/packages/api-fetch/src/index.js +++ b/packages/api-fetch/src/index.js @@ -9,6 +9,7 @@ import { __ } from '@wordpress/i18n'; import createNonceMiddleware from './middlewares/nonce'; import createRootURLMiddleware from './middlewares/root-url'; import createPreloadingMiddleware from './middlewares/preloading'; +import fetchAllMiddleware from './middlewares/fetch-all-middleware'; import namespaceEndpointMiddleware from './middlewares/namespace-endpoint'; import httpV1Middleware from './middlewares/http-v1'; @@ -107,6 +108,7 @@ function apiFetch( options ) { raw, httpV1Middleware, namespaceEndpointMiddleware, + fetchAllMiddleware, ...middlewares, ]; const next = ( nextOptions ) => { @@ -119,66 +121,6 @@ function apiFetch( options ) { apiFetch.use = registerMiddleware; -const capPerPageQuery = ( options ) => ( { - ...options, - // Swap back to requesting the max of 100 items per page. - // TODO: This feels brittle. Is there a better way to manage request parameters? - path: options.path && `${ options.path.replace( /(\&?per_page)=-1/, '$1=100' ) }&page=1`, - url: options.url && `${ options.url.replace( /(\&?per_page)=-1/, '$1=100' ) }&page=1`, -} ); - -const setRequestedPage = ( options, page ) => ( { - ...options, - path: options.path && options.path.replace( '&page=1', `&page=${ page }` ), - url: options.url && options.url.replace( '&page=1', `&page=${ page }` ), -} ); - -// The REST API enforces an upper limit of 100 on the per_page option. To handle -// large collections, apiFetch consumers can pass `per_page=-1` and this middleware -// function will recursively assemble a full response by paging over all available -// pages of API data. -const fetchAllMiddleware = async ( options, next ) => { - try { - if ( options.url && options.url.indexOf( 'per_page=-1' ) < 0 ) { - return next( options ); - } - - const pageOptions = capPerPageQuery( options ); - const pageOneResults = await next( { - ...pageOptions, - // Ensure headers are returned for page 1. - parse: false, - } ); - const pageOneParsed = await parseResponse( pageOneResults, options.parse ); - - const totalPages = pageOneResults.headers && pageOneResults.headers.get( 'X-WP-TotalPages' ); - - // Build an array of options objects for each remaining page. - const remainingPageRequests = Array.from( { - length: totalPages - 1, - } ).map( ( _, idx ) => { - // Specify the page to request. First additional request is index 0 but page 2, etc. - // Build these URLs based on pageOptions to avoid repeating the per_page adjustment. - return setRequestedPage( pageOptions, idx + 2 ); - } ); - - return remainingPageRequests.reduce( - // Request each remaining page in sequence, and return a merged array. - async ( previousPageRequest, nextPageOptions ) => { - const resultsCollection = await previousPageRequest; - const nextPage = await apiFetch( nextPageOptions ); - return resultsCollection.concat( nextPage ); - }, - // Ensure that the first page is parsed properly, as specified in the original options. - [].concat( pageOneParsed ) - ); - } catch ( e ) { - return Promise.reject( e ); - } -}; - -apiFetch.use( fetchAllMiddleware ); - apiFetch.createNonceMiddleware = createNonceMiddleware; apiFetch.createPreloadingMiddleware = createPreloadingMiddleware; apiFetch.createRootURLMiddleware = createRootURLMiddleware; diff --git a/packages/api-fetch/src/middlewares/fetch-all-middleware.js b/packages/api-fetch/src/middlewares/fetch-all-middleware.js new file mode 100644 index 00000000000000..0c9c060f5a5332 --- /dev/null +++ b/packages/api-fetch/src/middlewares/fetch-all-middleware.js @@ -0,0 +1,71 @@ +import apiFetch from '../'; + +// Duplicates parsing functionality from apiFetch. +function parseResponse( response, parse = true ) { + if ( parse ) { + return response.json ? response.json() : Promise.reject( response ); + } + + return response; +} + +const capPerPageQuery = ( options ) => ( { + ...options, + // Swap back to requesting the max of 100 items per page. + // TODO: This feels brittle. Is there a better way to manage request parameters? + path: options.path && `${ options.path.replace( /(\&?per_page)=-1/, '$1=100' ) }&page=1`, + url: options.url && `${ options.url.replace( /(\&?per_page)=-1/, '$1=100' ) }&page=1`, +} ); + +const setRequestedPage = ( options, page ) => ( { + ...options, + path: options.path && options.path.replace( '&page=1', `&page=${ page }` ), + url: options.url && options.url.replace( '&page=1', `&page=${ page }` ), +} ); + +// The REST API enforces an upper limit of 100 on the per_page option. To handle +// large collections, apiFetch consumers can pass `per_page=-1` and this middleware +// function will recursively assemble a full response by paging over all available +// pages of API data. +const fetchAllMiddleware = async ( options, next ) => { + try { + if ( options.url && options.url.indexOf( 'per_page=-1' ) < 0 ) { + return next( options ); + } + + const pageOptions = capPerPageQuery( options ); + const pageOneResults = await next( { + ...pageOptions, + // Ensure headers are returned for page 1. + parse: false, + } ); + const pageOneParsed = await parseResponse( pageOneResults, options.parse ); + + const totalPages = pageOneResults.headers && pageOneResults.headers.get( 'X-WP-TotalPages' ); + + // Build an array of options objects for each remaining page. + const remainingPageRequests = Array.from( { + length: totalPages - 1, + } ).map( ( _, idx ) => { + // Specify the page to request. First additional request is index 0 but page 2, etc. + // Build these URLs based on pageOptions to avoid repeating the per_page adjustment. + return setRequestedPage( pageOptions, idx + 2 ); + } ); + + return remainingPageRequests.reduce( + // Request each remaining page in sequence, and return a merged array. + async ( previousPageRequest, nextPageOptions ) => { + const resultsCollection = await previousPageRequest; + // TODO: Circular dependency + const nextPage = await apiFetch( nextPageOptions ); + return resultsCollection.concat( nextPage ); + }, + // Ensure that the first page is parsed properly, as specified in the original options. + [].concat( pageOneParsed ) + ); + } catch ( e ) { + return Promise.reject( e ); + } +}; + +export default fetchAllMiddleware; From 0b132846cc6cb44adc3f6333f1de80a951367f5c Mon Sep 17 00:00:00 2001 From: "K. Adam White" Date: Thu, 18 Oct 2018 17:54:51 -0400 Subject: [PATCH 03/21] Use @wordpress/url module to modify query strings within fetchAllMiddleware --- lib/client-assets.php | 2 +- packages/api-fetch/package.json | 3 ++- .../src/middlewares/fetch-all-middleware.js | 27 +++++++++++++++---- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/lib/client-assets.php b/lib/client-assets.php index 7f1e3b48938bbd..b9b2f10037262c 100644 --- a/lib/client-assets.php +++ b/lib/client-assets.php @@ -231,7 +231,7 @@ function gutenberg_register_scripts_and_styles() { gutenberg_override_script( 'wp-api-fetch', gutenberg_url( 'build/api-fetch/index.js' ), - array( 'wp-polyfill', 'wp-hooks', 'wp-i18n' ), + array( 'wp-polyfill', 'wp-hooks', 'wp-i18n', 'wp-url' ), filemtime( gutenberg_dir_path() . 'build/api-fetch/index.js' ), true ); diff --git a/packages/api-fetch/package.json b/packages/api-fetch/package.json index ed66f99b0444eb..bf5ed7b10f8f08 100644 --- a/packages/api-fetch/package.json +++ b/packages/api-fetch/package.json @@ -23,7 +23,8 @@ "dependencies": { "@babel/runtime": "^7.0.0", "@wordpress/hooks": "file:../hooks", - "@wordpress/i18n": "file:../i18n" + "@wordpress/i18n": "file:../i18n", + "@wordpress/url": "file:../url" }, "publishConfig": { "access": "public" diff --git a/packages/api-fetch/src/middlewares/fetch-all-middleware.js b/packages/api-fetch/src/middlewares/fetch-all-middleware.js index 0c9c060f5a5332..42a3cc7948a24f 100644 --- a/packages/api-fetch/src/middlewares/fetch-all-middleware.js +++ b/packages/api-fetch/src/middlewares/fetch-all-middleware.js @@ -1,3 +1,11 @@ +/** + * WordPress dependencies + */ +import { addQueryArgs } from '@wordpress/url'; + +/** + * Internal dependencies + */ import apiFetch from '../'; // Duplicates parsing functionality from apiFetch. @@ -12,15 +20,24 @@ function parseResponse( response, parse = true ) { const capPerPageQuery = ( options ) => ( { ...options, // Swap back to requesting the max of 100 items per page. - // TODO: This feels brittle. Is there a better way to manage request parameters? - path: options.path && `${ options.path.replace( /(\&?per_page)=-1/, '$1=100' ) }&page=1`, - url: options.url && `${ options.url.replace( /(\&?per_page)=-1/, '$1=100' ) }&page=1`, + path: options.path && addQueryArgs( options.path, { + per_page: 100, + page: 1, + } ), + url: options.url && addQueryArgs( options.url, { + per_page: 100, + page: 1, + } ), } ); const setRequestedPage = ( options, page ) => ( { ...options, - path: options.path && options.path.replace( '&page=1', `&page=${ page }` ), - url: options.url && options.url.replace( '&page=1', `&page=${ page }` ), + path: options.path && addQueryArgs( options.path, { + page, + } ), + url: options.url && addQueryArgs( options.url, { + page, + } ), } ); // The REST API enforces an upper limit of 100 on the per_page option. To handle From c54aa57fd6f0bf02d3f1474de26b24b55a25903e Mon Sep 17 00:00:00 2001 From: "K. Adam White" Date: Thu, 18 Oct 2018 22:26:52 -0400 Subject: [PATCH 04/21] Make fetchAllMiddleware optional within apiFetch and add it with inline script --- lib/client-assets.php | 5 +++++ packages/api-fetch/src/index.js | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/client-assets.php b/lib/client-assets.php index b9b2f10037262c..e7f00817f92b03 100644 --- a/lib/client-assets.php +++ b/lib/client-assets.php @@ -251,6 +251,11 @@ function gutenberg_register_scripts_and_styles() { ), 'after' ); + wp_add_inline_script( + 'wp-api-fetch', + 'wp.apiFetch.use( wp.apiFetch.fetchAllMiddleware );', + 'after' + ); gutenberg_override_script( 'wp-deprecated', diff --git a/packages/api-fetch/src/index.js b/packages/api-fetch/src/index.js index aa73db04820858..c00f0d6552d212 100644 --- a/packages/api-fetch/src/index.js +++ b/packages/api-fetch/src/index.js @@ -108,7 +108,6 @@ function apiFetch( options ) { raw, httpV1Middleware, namespaceEndpointMiddleware, - fetchAllMiddleware, ...middlewares, ]; const next = ( nextOptions ) => { @@ -124,5 +123,6 @@ apiFetch.use = registerMiddleware; apiFetch.createNonceMiddleware = createNonceMiddleware; apiFetch.createPreloadingMiddleware = createPreloadingMiddleware; apiFetch.createRootURLMiddleware = createRootURLMiddleware; +apiFetch.fetchAllMiddleware = fetchAllMiddleware; export default apiFetch; From ed69cb59ff8af2c312e4efb0898954cfa79e09d1 Mon Sep 17 00:00:00 2001 From: "K. Adam White" Date: Thu, 18 Oct 2018 23:27:08 -0400 Subject: [PATCH 05/21] Improve comments for fetchAllMiddleware module --- .../src/middlewares/fetch-all-middleware.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/api-fetch/src/middlewares/fetch-all-middleware.js b/packages/api-fetch/src/middlewares/fetch-all-middleware.js index 42a3cc7948a24f..ffaad8f5ac6f58 100644 --- a/packages/api-fetch/src/middlewares/fetch-all-middleware.js +++ b/packages/api-fetch/src/middlewares/fetch-all-middleware.js @@ -19,7 +19,7 @@ function parseResponse( response, parse = true ) { const capPerPageQuery = ( options ) => ( { ...options, - // Swap back to requesting the max of 100 items per page. + // Swap back to requesting the max number of items per page. path: options.path && addQueryArgs( options.path, { per_page: 100, page: 1, @@ -40,10 +40,9 @@ const setRequestedPage = ( options, page ) => ( { } ), } ); -// The REST API enforces an upper limit of 100 on the per_page option. To handle -// large collections, apiFetch consumers can pass `per_page=-1` and this middleware -// function will recursively assemble a full response by paging over all available -// pages of API data. +// The REST API enforces an upper limit on the per_page option. To handle large +// collections, apiFetch consumers can pass `per_page=-1`; this middleware will +// then recursively assemble a full response array from all available pages. const fetchAllMiddleware = async ( options, next ) => { try { if ( options.url && options.url.indexOf( 'per_page=-1' ) < 0 ) { @@ -56,7 +55,6 @@ const fetchAllMiddleware = async ( options, next ) => { // Ensure headers are returned for page 1. parse: false, } ); - const pageOneParsed = await parseResponse( pageOneResults, options.parse ); const totalPages = pageOneResults.headers && pageOneResults.headers.get( 'X-WP-TotalPages' ); @@ -65,19 +63,19 @@ const fetchAllMiddleware = async ( options, next ) => { length: totalPages - 1, } ).map( ( _, idx ) => { // Specify the page to request. First additional request is index 0 but page 2, etc. - // Build these URLs based on pageOptions to avoid repeating the per_page adjustment. return setRequestedPage( pageOptions, idx + 2 ); } ); + // Ensure the first page is parsed as specified. + const pageOneParsed = await parseResponse( pageOneResults, options.parse ); + return remainingPageRequests.reduce( // Request each remaining page in sequence, and return a merged array. async ( previousPageRequest, nextPageOptions ) => { const resultsCollection = await previousPageRequest; - // TODO: Circular dependency const nextPage = await apiFetch( nextPageOptions ); return resultsCollection.concat( nextPage ); }, - // Ensure that the first page is parsed properly, as specified in the original options. [].concat( pageOneParsed ) ); } catch ( e ) { From 49d80cdbd15068306db57cbd87aa808467d3e908 Mon Sep 17 00:00:00 2001 From: "K. Adam White" Date: Fri, 19 Oct 2018 15:33:27 -0400 Subject: [PATCH 06/21] Improve fetchAllMiddleware: use links for pagination. - Remove unneeded modification to apiFetch parsing logic. - Utilize the links header to traverse collection pages. - Remove confusing try/catch behavior. - Add more escape hatches for cases where middleware should not apply. --- packages/api-fetch/src/index.js | 16 +-- .../src/middlewares/fetch-all-middleware.js | 133 ++++++++++-------- 2 files changed, 82 insertions(+), 67 deletions(-) diff --git a/packages/api-fetch/src/index.js b/packages/api-fetch/src/index.js index c00f0d6552d212..bbea7c3d1378fb 100644 --- a/packages/api-fetch/src/index.js +++ b/packages/api-fetch/src/index.js @@ -27,14 +27,6 @@ function checkCloudflareError( error ) { } } -function parseResponse( response, parse = true ) { - if ( parse ) { - return response.json ? response.json() : Promise.reject( response ); - } - - return response; -} - function apiFetch( options ) { const raw = ( nextOptions ) => { const { url, path, body, data, parse, ...remainingOptions } = nextOptions; @@ -62,7 +54,13 @@ function apiFetch( options ) { return responsePromise .then( checkStatus ) - .then( ( response ) => parseResponse( response, parse ) ) + .then( ( response ) => { + if ( parse ) { + return response.json ? response.json() : Promise.reject( response ); + } + + return response; + } ) .catch( ( response ) => { if ( ! parse ) { throw response; diff --git a/packages/api-fetch/src/middlewares/fetch-all-middleware.js b/packages/api-fetch/src/middlewares/fetch-all-middleware.js index ffaad8f5ac6f58..4d427523fa8909 100644 --- a/packages/api-fetch/src/middlewares/fetch-all-middleware.js +++ b/packages/api-fetch/src/middlewares/fetch-all-middleware.js @@ -1,3 +1,4 @@ +/* eslint-disable no-console */ /** * WordPress dependencies */ @@ -8,79 +9,95 @@ import { addQueryArgs } from '@wordpress/url'; */ import apiFetch from '../'; +// Apply query arguments to both URL and Path, whichever is present. +const modifyQuery = ( { path, url, ...options }, queryArgs ) => ( { + ...options, + ...( url ? { + url: addQueryArgs( url || path, queryArgs ), + } : {} ), + ...( path ? { + path: addQueryArgs( path || path, queryArgs ), + } : {} ), +} ); + // Duplicates parsing functionality from apiFetch. -function parseResponse( response, parse = true ) { - if ( parse ) { - return response.json ? response.json() : Promise.reject( response ); - } +const parseResponse = ( response ) => response.json ? + response.json() : + Promise.reject( response ); - return response; -} +const parseLinkHeader = ( linkHeader ) => { + if ( ! linkHeader ) { + return {}; + } + const match = linkHeader.match( /<([^>]+)>; rel="next"/ ); + return match ? { + next: match[ 1 ], + } : {}; +}; -const capPerPageQuery = ( options ) => ( { - ...options, - // Swap back to requesting the max number of items per page. - path: options.path && addQueryArgs( options.path, { - per_page: 100, - page: 1, - } ), - url: options.url && addQueryArgs( options.url, { - per_page: 100, - page: 1, - } ), -} ); +const getNextPageUrl = ( response ) => { + const { next } = parseLinkHeader( response.headers.get( 'link' ) ); + return next; +}; -const setRequestedPage = ( options, page ) => ( { - ...options, - path: options.path && addQueryArgs( options.path, { - page, - } ), - url: options.url && addQueryArgs( options.url, { - page, - } ), -} ); +const requestContainsUnboundedQuery = ( options ) => { + const pathIsUnbounded = options.path && options.path.indexOf( 'per_page=-1' > -1 ); + const urlIsUnbounded = options.url && options.url.indexOf( 'per_page=-1' > -1 ); + return pathIsUnbounded || urlIsUnbounded; +}; // The REST API enforces an upper limit on the per_page option. To handle large // collections, apiFetch consumers can pass `per_page=-1`; this middleware will // then recursively assemble a full response array from all available pages. const fetchAllMiddleware = async ( options, next ) => { - try { - if ( options.url && options.url.indexOf( 'per_page=-1' ) < 0 ) { - return next( options ); - } + if ( options.parse === false ) { + // If a consumer has opted out of parsing, do not apply middleware. + return next( options ); + } + if ( ! requestContainsUnboundedQuery( options ) ) { + // If neither url nor path is requesting all items, do not apply middleware. + return next( options ); + } - const pageOptions = capPerPageQuery( options ); - const pageOneResults = await next( { - ...pageOptions, - // Ensure headers are returned for page 1. - parse: false, - } ); + // Retrieve requested page of results. + const response = await next( { + ...modifyQuery( options, { + per_page: 100, + } ), + // Ensure headers are returned for page 1. + parse: false, + } ); - const totalPages = pageOneResults.headers && pageOneResults.headers.get( 'X-WP-TotalPages' ); + const results = await parseResponse( response ); - // Build an array of options objects for each remaining page. - const remainingPageRequests = Array.from( { - length: totalPages - 1, - } ).map( ( _, idx ) => { - // Specify the page to request. First additional request is index 0 but page 2, etc. - return setRequestedPage( pageOptions, idx + 2 ); - } ); + if ( ! Array.isArray( results ) ) { + // We have no reliable way of merging non-array results. + return results; + } + + let nextPage = getNextPageUrl( response ); - // Ensure the first page is parsed as specified. - const pageOneParsed = await parseResponse( pageOneResults, options.parse ); + if ( ! nextPage ) { + // There are no further pages to request. + return results; + } - return remainingPageRequests.reduce( - // Request each remaining page in sequence, and return a merged array. - async ( previousPageRequest, nextPageOptions ) => { - const resultsCollection = await previousPageRequest; - const nextPage = await apiFetch( nextPageOptions ); - return resultsCollection.concat( nextPage ); - }, - [].concat( pageOneParsed ) - ); - } catch ( e ) { - return Promise.reject( e ); + // Iteratively fetch all remaining pages until no "next" header is found. + let mergedResults = [].concat( results ); + while ( nextPage ) { + const nextResponse = await apiFetch( { + ...options, + // Ensure the URL for the next page is used instead of any provided path. + path: undefined, + url: nextPage, + // Ensure we still get headers so we can identify the next page. + parse: false, + } ); + const nextResults = await parseResponse( nextResponse ); + mergedResults = mergedResults.concat( nextResults ); + nextPage = getNextPageUrl( nextResponse ); } + return mergedResults; }; export default fetchAllMiddleware; From c615efd2811d5b0b12dcaacb0892249bbc0e14eb Mon Sep 17 00:00:00 2001 From: "K. Adam White" Date: Fri, 19 Oct 2018 15:39:12 -0400 Subject: [PATCH 07/21] Remove leftover debugging comment --- packages/api-fetch/src/middlewares/fetch-all-middleware.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/api-fetch/src/middlewares/fetch-all-middleware.js b/packages/api-fetch/src/middlewares/fetch-all-middleware.js index 4d427523fa8909..0c9dfd30a55c0b 100644 --- a/packages/api-fetch/src/middlewares/fetch-all-middleware.js +++ b/packages/api-fetch/src/middlewares/fetch-all-middleware.js @@ -1,4 +1,3 @@ -/* eslint-disable no-console */ /** * WordPress dependencies */ From d122d8753cd24f91eb4545d8cb0e6046e8c7544a Mon Sep 17 00:00:00 2001 From: "K. Adam White" Date: Fri, 19 Oct 2018 15:44:35 -0400 Subject: [PATCH 08/21] Apply fetchAllMiddleware within apiFetch, not client-scripts.php --- lib/client-assets.php | 5 ----- packages/api-fetch/src/index.js | 1 + 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/client-assets.php b/lib/client-assets.php index e7f00817f92b03..b9b2f10037262c 100644 --- a/lib/client-assets.php +++ b/lib/client-assets.php @@ -251,11 +251,6 @@ function gutenberg_register_scripts_and_styles() { ), 'after' ); - wp_add_inline_script( - 'wp-api-fetch', - 'wp.apiFetch.use( wp.apiFetch.fetchAllMiddleware );', - 'after' - ); gutenberg_override_script( 'wp-deprecated', diff --git a/packages/api-fetch/src/index.js b/packages/api-fetch/src/index.js index bbea7c3d1378fb..f63448607a541a 100644 --- a/packages/api-fetch/src/index.js +++ b/packages/api-fetch/src/index.js @@ -104,6 +104,7 @@ function apiFetch( options ) { const steps = [ raw, + fetchAllMiddleware, httpV1Middleware, namespaceEndpointMiddleware, ...middlewares, From e11f34c7fe4716b8f919c927a34d3793b7c592d4 Mon Sep 17 00:00:00 2001 From: "K. Adam White" Date: Fri, 19 Oct 2018 15:44:51 -0400 Subject: [PATCH 09/21] Simplify obtuse code in query modification logic --- .../api-fetch/src/middlewares/fetch-all-middleware.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/api-fetch/src/middlewares/fetch-all-middleware.js b/packages/api-fetch/src/middlewares/fetch-all-middleware.js index 0c9dfd30a55c0b..24a67a3633e191 100644 --- a/packages/api-fetch/src/middlewares/fetch-all-middleware.js +++ b/packages/api-fetch/src/middlewares/fetch-all-middleware.js @@ -11,12 +11,8 @@ import apiFetch from '../'; // Apply query arguments to both URL and Path, whichever is present. const modifyQuery = ( { path, url, ...options }, queryArgs ) => ( { ...options, - ...( url ? { - url: addQueryArgs( url || path, queryArgs ), - } : {} ), - ...( path ? { - path: addQueryArgs( path || path, queryArgs ), - } : {} ), + url: url && addQueryArgs( url, queryArgs ), + path: path && addQueryArgs( path, queryArgs ), } ); // Duplicates parsing functionality from apiFetch. From 7703ce2d4082a81794b6c03908e36aaf85a34d6f Mon Sep 17 00:00:00 2001 From: "K. Adam White" Date: Fri, 19 Oct 2018 16:09:05 -0400 Subject: [PATCH 10/21] Fix improperly reverted parsing code change --- packages/api-fetch/src/index.js | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/packages/api-fetch/src/index.js b/packages/api-fetch/src/index.js index f63448607a541a..4f6b3ce202645a 100644 --- a/packages/api-fetch/src/index.js +++ b/packages/api-fetch/src/index.js @@ -29,7 +29,7 @@ function checkCloudflareError( error ) { function apiFetch( options ) { const raw = ( nextOptions ) => { - const { url, path, body, data, parse, ...remainingOptions } = nextOptions; + const { url, path, body, data, parse = true, ...remainingOptions } = nextOptions; const headers = remainingOptions.headers || {}; if ( ! headers[ 'Content-Type' ] && data ) { headers[ 'Content-Type' ] = 'application/json'; @@ -52,15 +52,17 @@ function apiFetch( options ) { throw response; }; + const parseResponse = ( response ) => { + if ( parse ) { + return response.json ? response.json() : Promise.reject( response ); + } + + return response; + }; + return responsePromise .then( checkStatus ) - .then( ( response ) => { - if ( parse ) { - return response.json ? response.json() : Promise.reject( response ); - } - - return response; - } ) + .then( parseResponse ) .catch( ( response ) => { if ( ! parse ) { throw response; From c2c2f6cec00c38fd8c3629b61b4b238d2a6b56f2 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Sun, 21 Oct 2018 15:31:10 +0100 Subject: [PATCH 11/21] Fix unit tests --- packages/api-fetch/src/middlewares/fetch-all-middleware.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/api-fetch/src/middlewares/fetch-all-middleware.js b/packages/api-fetch/src/middlewares/fetch-all-middleware.js index 24a67a3633e191..83e2ef08402d21 100644 --- a/packages/api-fetch/src/middlewares/fetch-all-middleware.js +++ b/packages/api-fetch/src/middlewares/fetch-all-middleware.js @@ -36,8 +36,8 @@ const getNextPageUrl = ( response ) => { }; const requestContainsUnboundedQuery = ( options ) => { - const pathIsUnbounded = options.path && options.path.indexOf( 'per_page=-1' > -1 ); - const urlIsUnbounded = options.url && options.url.indexOf( 'per_page=-1' > -1 ); + const pathIsUnbounded = options.path && options.path.indexOf( 'per_page=-1' ) !== -1; + const urlIsUnbounded = options.url && options.url.indexOf( 'per_page=-1' ) !== -1; return pathIsUnbounded || urlIsUnbounded; }; From 158f088439d0da54befe2a360e41cb653ca319e0 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Sun, 21 Oct 2018 15:35:08 +0100 Subject: [PATCH 12/21] Avoid circular dependency --- packages/api-fetch/src/middlewares/fetch-all-middleware.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/api-fetch/src/middlewares/fetch-all-middleware.js b/packages/api-fetch/src/middlewares/fetch-all-middleware.js index 83e2ef08402d21..5bc17b5413d326 100644 --- a/packages/api-fetch/src/middlewares/fetch-all-middleware.js +++ b/packages/api-fetch/src/middlewares/fetch-all-middleware.js @@ -3,11 +3,6 @@ */ import { addQueryArgs } from '@wordpress/url'; -/** - * Internal dependencies - */ -import apiFetch from '../'; - // Apply query arguments to both URL and Path, whichever is present. const modifyQuery = ( { path, url, ...options }, queryArgs ) => ( { ...options, @@ -80,7 +75,7 @@ const fetchAllMiddleware = async ( options, next ) => { // Iteratively fetch all remaining pages until no "next" header is found. let mergedResults = [].concat( results ); while ( nextPage ) { - const nextResponse = await apiFetch( { + const nextResponse = await next( { ...options, // Ensure the URL for the next page is used instead of any provided path. path: undefined, From 5c7a2f2224fdd2f4cd610a2fd55ee7ad61e2965d Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Sun, 21 Oct 2018 15:50:36 +0100 Subject: [PATCH 13/21] Adding some unit tests --- .../middlewares/test/fetch-all-middleware.js | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 packages/api-fetch/src/middlewares/test/fetch-all-middleware.js diff --git a/packages/api-fetch/src/middlewares/test/fetch-all-middleware.js b/packages/api-fetch/src/middlewares/test/fetch-all-middleware.js new file mode 100644 index 00000000000000..930a7d7da970cc --- /dev/null +++ b/packages/api-fetch/src/middlewares/test/fetch-all-middleware.js @@ -0,0 +1,38 @@ +/** + * Internal dependencies + */ +import fetchAllMiddleware from '../fetch-all-middleware'; + +describe( 'Fetch All Middleware', async () => { + it( 'should defer with the same options to the next middleware', async () => { + expect.hasAssertions(); + const originalOptions = { path: '/posts' }; + const next = ( options ) => { + expect( options ).toBe( originalOptions ); + return Promise.resolve( 'ok' ); + }; + + await fetchAllMiddleware( originalOptions, next ); + } ); + + it( 'should paginate the request', async () => { + expect.hasAssertions(); + const originalOptions = { path: '/posts?per_page=-1' }; + const next = ( options ) => { + expect( options.path ).toBe( '/posts?per_page=100' ); + return Promise.resolve( { + status: 200, + headers: { + get() { + return ''; + }, + }, + json() { + return Promise.resolve( { message: 'ok' } ); + }, + } ); + }; + + await fetchAllMiddleware( originalOptions, next ); + } ); +} ); From b7bb2cd43e6a7b56e346292e0865145c447532aa Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Sun, 21 Oct 2018 15:54:50 +0100 Subject: [PATCH 14/21] Remove useless per_page argument --- packages/edit-post/src/hooks/components/media-upload/index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/edit-post/src/hooks/components/media-upload/index.js b/packages/edit-post/src/hooks/components/media-upload/index.js index 72e223ce66bd52..0e22b6cd5286ec 100644 --- a/packages/edit-post/src/hooks/components/media-upload/index.js +++ b/packages/edit-post/src/hooks/components/media-upload/index.js @@ -67,7 +67,6 @@ const getAttachmentsCollection = ( ids ) => { return wp.media.query( { order: 'ASC', orderby: 'post__in', - per_page: -1, post__in: ids, query: true, type: 'image', From 5436e18a9c4f88488dde947b119958e91afb3d7d Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Sun, 21 Oct 2018 16:06:38 +0100 Subject: [PATCH 15/21] Update the lock file --- package-lock.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/package-lock.json b/package-lock.json index 1867236d23dc43..179915339a81b5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -2007,7 +2007,8 @@ "requires": { "@babel/runtime": "^7.0.0", "@wordpress/hooks": "file:packages/hooks", - "@wordpress/i18n": "file:packages/i18n" + "@wordpress/i18n": "file:packages/i18n", + "@wordpress/url": "file:packages/url" } }, "@wordpress/autop": { From 8803b88e7e7eed8f73aba32b52608175b1a2e858 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Sun, 21 Oct 2018 16:08:14 +0100 Subject: [PATCH 16/21] Update api-fetch CHANGELOG --- packages/api-fetch/CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/api-fetch/CHANGELOG.md b/packages/api-fetch/CHANGELOG.md index a49cefcd0d75cf..7d53f6aa2222f8 100644 --- a/packages/api-fetch/CHANGELOG.md +++ b/packages/api-fetch/CHANGELOG.md @@ -1,3 +1,7 @@ +## 2.1.0 (Unreleased) + +- Support `per_page=-1` paginated requests. + ## 2.0.0 (2018-09-05) ### Breaking Change From 0a08388f8d744dc2688181d870829484b3467de0 Mon Sep 17 00:00:00 2001 From: Daniel Bachhuber Date: Sun, 21 Oct 2018 08:20:43 -0700 Subject: [PATCH 17/21] Fetch as many attachments as we can, even if its not all of them --- packages/edit-post/src/hooks/components/media-upload/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/edit-post/src/hooks/components/media-upload/index.js b/packages/edit-post/src/hooks/components/media-upload/index.js index 0e22b6cd5286ec..f950d34d628eee 100644 --- a/packages/edit-post/src/hooks/components/media-upload/index.js +++ b/packages/edit-post/src/hooks/components/media-upload/index.js @@ -68,6 +68,7 @@ const getAttachmentsCollection = ( ids ) => { order: 'ASC', orderby: 'post__in', post__in: ids, + per_page: 100, query: true, type: 'image', } ); From 1ce1f2b922de8f3e3c4e574acb32b6d8bedc60d7 Mon Sep 17 00:00:00 2001 From: Daniel Bachhuber Date: Sun, 21 Oct 2018 08:22:11 -0700 Subject: [PATCH 18/21] Remove `per_page=-1` support shim in REST API --- lib/rest-api.php | 148 ---------------------- phpunit/class-gutenberg-rest-api-test.php | 38 +----- 2 files changed, 3 insertions(+), 183 deletions(-) diff --git a/lib/rest-api.php b/lib/rest-api.php index 7506f45484ee79..35f4af5c52e77e 100644 --- a/lib/rest-api.php +++ b/lib/rest-api.php @@ -290,158 +290,10 @@ function gutenberg_register_post_prepare_functions( $post_type ) { add_filter( "rest_prepare_{$post_type}", 'gutenberg_add_permalink_template_to_posts', 10, 3 ); add_filter( "rest_prepare_{$post_type}", 'gutenberg_add_block_format_to_post_content', 10, 3 ); add_filter( "rest_prepare_{$post_type}", 'gutenberg_add_target_schema_to_links', 10, 3 ); - add_filter( "rest_{$post_type}_collection_params", 'gutenberg_filter_post_collection_parameters', 10, 2 ); - add_filter( "rest_{$post_type}_query", 'gutenberg_filter_post_query_arguments', 10, 2 ); return $post_type; } add_filter( 'registered_post_type', 'gutenberg_register_post_prepare_functions' ); -/** - * Whenever a taxonomy is registered, ensure we're hooked into its WP REST API response. - * - * @param string $taxonomy The newly registered taxonomy. - */ -function gutenberg_register_taxonomy_prepare_functions( $taxonomy ) { - add_filter( "rest_{$taxonomy}_collection_params", 'gutenberg_filter_term_collection_parameters', 10, 2 ); - add_filter( "rest_{$taxonomy}_query", 'gutenberg_filter_term_query_arguments', 10, 2 ); -} -add_filter( 'registered_taxonomy', 'gutenberg_register_taxonomy_prepare_functions' ); - -/** - * Handle any necessary checks early. - * - * @param WP_HTTP_Response $response Result to send to the client. Usually a WP_REST_Response. - * @param WP_REST_Server $handler ResponseHandler instance (usually WP_REST_Server). - * @param WP_REST_Request $request Request used to generate the response. - */ -function gutenberg_handle_early_callback_checks( $response, $handler, $request ) { - if ( 0 === strpos( $request->get_route(), '/wp/v2/' ) ) { - $can_unbounded_query = false; - $types = get_post_types( array( 'show_in_rest' => true ), 'objects' ); - foreach ( $types as $type ) { - if ( current_user_can( $type->cap->edit_posts ) ) { - $can_unbounded_query = true; - } - } - if ( $request['per_page'] < 0 ) { - if ( ! $can_unbounded_query ) { - return new WP_Error( 'rest_forbidden_per_page', __( 'Sorry, you are not allowed to make unbounded queries.', 'gutenberg' ), array( 'status' => rest_authorization_required_code() ) ); - } - } - } - return $response; -} -add_filter( 'rest_request_before_callbacks', 'gutenberg_handle_early_callback_checks', 10, 3 ); - -/** - * Include additional query parameters on the posts query endpoint. - * - * @see https://core.trac.wordpress.org/ticket/43998 - * - * @param array $query_params JSON Schema-formatted collection parameters. - * @param WP_Post_Type $post_type Post type object being accessed. - * @return array - */ -function gutenberg_filter_post_collection_parameters( $query_params, $post_type ) { - if ( - isset( $query_params['per_page'] ) && - ( $post_type->hierarchical || 'wp_block' === $post_type->name ) - ) { - // Change from '1' to '-1', which means unlimited. - $query_params['per_page']['minimum'] = -1; - // Default sanitize callback is 'absint', which won't work in our case. - $query_params['per_page']['sanitize_callback'] = 'rest_sanitize_request_arg'; - } - return $query_params; -} - -/** - * Filter post collection query parameters to include specific behavior. - * - * @see https://core.trac.wordpress.org/ticket/43998 - * - * @param array $prepared_args Array of arguments for WP_Query. - * @param WP_REST_Request $request The current request. - * @return array - */ -function gutenberg_filter_post_query_arguments( $prepared_args, $request ) { - if ( - is_post_type_hierarchical( $prepared_args['post_type'] ) || - 'wp_block' === $prepared_args['post_type'] - ) { - // Avoid triggering 'rest_post_invalid_page_number' error - // which will need to be addressed in https://core.trac.wordpress.org/ticket/43998. - if ( -1 === $prepared_args['posts_per_page'] ) { - $prepared_args['posts_per_page'] = 100000; - } - } - return $prepared_args; -} - -/** - * Include additional query parameters on the terms query endpoint. - * - * @see https://core.trac.wordpress.org/ticket/43998 - * - * @param array $query_params JSON Schema-formatted collection parameters. - * @param object $taxonomy Taxonomy being accessed. - * @return array - */ -function gutenberg_filter_term_collection_parameters( $query_params, $taxonomy ) { - if ( $taxonomy->show_in_rest - && ( false === $taxonomy->rest_controller_class - || 'WP_REST_Terms_Controller' === $taxonomy->rest_controller_class ) - && isset( $query_params['per_page'] ) ) { - // Change from '1' to '-1', which means unlimited. - $query_params['per_page']['minimum'] = -1; - // Default sanitize callback is 'absint', which won't work in our case. - $query_params['per_page']['sanitize_callback'] = 'rest_sanitize_request_arg'; - } - return $query_params; -} - -/** - * Filter term collection query parameters to include specific behavior. - * - * @see https://core.trac.wordpress.org/ticket/43998 - * - * @param array $prepared_args Array of arguments for WP_Term_Query. - * @param WP_REST_Request $request The current request. - * @return array - */ -function gutenberg_filter_term_query_arguments( $prepared_args, $request ) { - // Can't check the actual taxonomy here because it's not - // passed through in $prepared_args (or the filter generally). - if ( 0 === strpos( $request->get_route(), '/wp/v2/' ) ) { - if ( -1 === $prepared_args['number'] ) { - // This should be unset( $prepared_args['number'] ) - // but WP_REST_Terms Controller needs to be updated to support - // unbounded queries. - // Will be addressed in https://core.trac.wordpress.org/ticket/43998. - $prepared_args['number'] = 100000; - } - } - return $prepared_args; -} - -/** - * Include additional query parameters on the user query endpoint. - * - * @see https://core.trac.wordpress.org/ticket/43998 - * - * @param array $query_params JSON Schema-formatted collection parameters. - * @return array - */ -function gutenberg_filter_user_collection_parameters( $query_params ) { - if ( isset( $query_params['per_page'] ) ) { - // Change from '1' to '-1', which means unlimited. - $query_params['per_page']['minimum'] = -1; - // Default sanitize callback is 'absint', which won't work in our case. - $query_params['per_page']['sanitize_callback'] = 'rest_sanitize_request_arg'; - } - return $query_params; -} -add_filter( 'rest_user_collection_params', 'gutenberg_filter_user_collection_parameters' ); /** * Silence PHP Warnings and Errors in JSON requests diff --git a/phpunit/class-gutenberg-rest-api-test.php b/phpunit/class-gutenberg-rest-api-test.php index 6dc323d6179bd7..293dc7fed3526c 100644 --- a/phpunit/class-gutenberg-rest-api-test.php +++ b/phpunit/class-gutenberg-rest-api-test.php @@ -381,17 +381,7 @@ public function test_get_items_unbounded_per_page() { $request = new WP_REST_Request( 'GET', '/wp/v2/users' ); $request->set_param( 'per_page', '-1' ); $response = rest_get_server()->dispatch( $request ); - $this->assertEquals( 200, $response->get_status() ); - } - - public function test_get_items_unbounded_per_page_unauthorized() { - wp_set_current_user( $this->subscriber ); - $request = new WP_REST_Request( 'GET', '/wp/v2/users' ); - $request->set_param( 'per_page', '-1' ); - $response = rest_get_server()->dispatch( $request ); - $this->assertEquals( 403, $response->get_status() ); - $data = $response->get_data(); - $this->assertEquals( 'rest_forbidden_per_page', $data['code'] ); + $this->assertEquals( 400, $response->get_status() ); } public function test_get_categories_unbounded_per_page() { @@ -400,18 +390,7 @@ public function test_get_categories_unbounded_per_page() { $request = new WP_REST_Request( 'GET', '/wp/v2/categories' ); $request->set_param( 'per_page', '-1' ); $response = rest_get_server()->dispatch( $request ); - $this->assertEquals( 200, $response->get_status() ); - } - - public function test_get_categories_unbounded_per_page_unauthorized() { - wp_set_current_user( $this->subscriber ); - $this->factory->category->create(); - $request = new WP_REST_Request( 'GET', '/wp/v2/categories' ); - $request->set_param( 'per_page', '-1' ); - $response = rest_get_server()->dispatch( $request ); - $this->assertEquals( 403, $response->get_status() ); - $data = $response->get_data(); - $this->assertEquals( 'rest_forbidden_per_page', $data['code'] ); + $this->assertEquals( 400, $response->get_status() ); } public function test_get_pages_unbounded_per_page() { @@ -420,18 +399,7 @@ public function test_get_pages_unbounded_per_page() { $request = new WP_REST_Request( 'GET', '/wp/v2/pages' ); $request->set_param( 'per_page', '-1' ); $response = rest_get_server()->dispatch( $request ); - $this->assertEquals( 200, $response->get_status() ); - } - - public function test_get_pages_unbounded_per_page_unauthorized() { - wp_set_current_user( $this->subscriber ); - $this->factory->post->create( array( 'post_type' => 'page' ) ); - $request = new WP_REST_Request( 'GET', '/wp/v2/pages' ); - $request->set_param( 'per_page', '-1' ); - $response = rest_get_server()->dispatch( $request ); - $this->assertEquals( 403, $response->get_status() ); - $data = $response->get_data(); - $this->assertEquals( 'rest_forbidden_per_page', $data['code'] ); + $this->assertEquals( 400, $response->get_status() ); } public function test_get_post_links_predecessor_version() { From 93eeecc79e9ace3dc0c70f42d4a621a8d39a032f Mon Sep 17 00:00:00 2001 From: Daniel Bachhuber Date: Sun, 21 Oct 2018 09:00:45 -0700 Subject: [PATCH 19/21] Ensure the next page of results is actually fetched --- packages/api-fetch/src/middlewares/fetch-all-middleware.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/api-fetch/src/middlewares/fetch-all-middleware.js b/packages/api-fetch/src/middlewares/fetch-all-middleware.js index 5bc17b5413d326..8cc2f9b603f66b 100644 --- a/packages/api-fetch/src/middlewares/fetch-all-middleware.js +++ b/packages/api-fetch/src/middlewares/fetch-all-middleware.js @@ -1,3 +1,8 @@ +/** + * Internal dependencies + */ +import apiFetch from '../'; + /** * WordPress dependencies */ @@ -75,7 +80,7 @@ const fetchAllMiddleware = async ( options, next ) => { // Iteratively fetch all remaining pages until no "next" header is found. let mergedResults = [].concat( results ); while ( nextPage ) { - const nextResponse = await next( { + const nextResponse = await apiFetch( { ...options, // Ensure the URL for the next page is used instead of any provided path. path: undefined, From 08e8ca89c0c7520e1bdc3e4e7d9701c8bd0381a1 Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Sun, 21 Oct 2018 17:13:51 +0100 Subject: [PATCH 20/21] Tweak the middleware chain to avoid mutations --- packages/api-fetch/src/index.js | 10 ++++++---- .../api-fetch/src/middlewares/fetch-all-middleware.js | 7 +------ 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/packages/api-fetch/src/index.js b/packages/api-fetch/src/index.js index 4f6b3ce202645a..b028fef611e4b9 100644 --- a/packages/api-fetch/src/index.js +++ b/packages/api-fetch/src/index.js @@ -110,13 +110,15 @@ function apiFetch( options ) { httpV1Middleware, namespaceEndpointMiddleware, ...middlewares, - ]; - const next = ( nextOptions ) => { - const nextMiddleware = steps.pop(); + ].reverse(); + + const runMiddleware = ( index ) => ( nextOptions ) => { + const nextMiddleware = steps[ index ]; + const next = runMiddleware( index + 1 ); return nextMiddleware( nextOptions, next ); }; - return next( options ); + return runMiddleware( 0 )( options ); } apiFetch.use = registerMiddleware; diff --git a/packages/api-fetch/src/middlewares/fetch-all-middleware.js b/packages/api-fetch/src/middlewares/fetch-all-middleware.js index 8cc2f9b603f66b..5bc17b5413d326 100644 --- a/packages/api-fetch/src/middlewares/fetch-all-middleware.js +++ b/packages/api-fetch/src/middlewares/fetch-all-middleware.js @@ -1,8 +1,3 @@ -/** - * Internal dependencies - */ -import apiFetch from '../'; - /** * WordPress dependencies */ @@ -80,7 +75,7 @@ const fetchAllMiddleware = async ( options, next ) => { // Iteratively fetch all remaining pages until no "next" header is found. let mergedResults = [].concat( results ); while ( nextPage ) { - const nextResponse = await apiFetch( { + const nextResponse = await next( { ...options, // Ensure the URL for the next page is used instead of any provided path. path: undefined, From 562250dd5fe32a9b0a5e3df8767a578a71dc476b Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Sun, 21 Oct 2018 17:25:47 +0100 Subject: [PATCH 21/21] Testing fetching consequential pages --- .../middlewares/test/fetch-all-middleware.js | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/packages/api-fetch/src/middlewares/test/fetch-all-middleware.js b/packages/api-fetch/src/middlewares/test/fetch-all-middleware.js index 930a7d7da970cc..c002790ead45c3 100644 --- a/packages/api-fetch/src/middlewares/test/fetch-all-middleware.js +++ b/packages/api-fetch/src/middlewares/test/fetch-all-middleware.js @@ -17,22 +17,35 @@ describe( 'Fetch All Middleware', async () => { it( 'should paginate the request', async () => { expect.hasAssertions(); - const originalOptions = { path: '/posts?per_page=-1' }; + const originalOptions = { url: '/posts?per_page=-1' }; + let counter = 1; const next = ( options ) => { - expect( options.path ).toBe( '/posts?per_page=100' ); - return Promise.resolve( { + if ( counter === 1 ) { + expect( options.url ).toBe( '/posts?per_page=100' ); + } else { + expect( options.url ).toBe( '/posts?per_page=100&page=2' ); + } + const response = Promise.resolve( { status: 200, headers: { get() { - return ''; + return options.url === '/posts?per_page=100' ? + '; rel="next"' : + ''; }, }, json() { - return Promise.resolve( { message: 'ok' } ); + return Promise.resolve( [ 'item' ] ); }, } ); + + counter++; + + return response; }; - await fetchAllMiddleware( originalOptions, next ); + const result = await fetchAllMiddleware( originalOptions, next ); + + expect( result ).toEqual( [ 'item', 'item' ] ); } ); } );