Add pagination to search endpoints #583
Conversation
| @@ -91,11 +91,11 @@ class GitHub { | |||
|
|
|||
| /** | |||
| * Create a new Search wrapper | |||
| * @param {string} query - the query to search for | |||
| * @param {Search.Params} searchParameters - the query and other search parameters | |||
There was a problem hiding this comment.
Not sure if jsdoc understands how to reach into other files (Search.js). Let me know if this will be a problem.
| @@ -256,19 +273,19 @@ class Requestable { | |||
| results.push(...thisGroup); | |||
|
|
|||
| const nextUrl = getNextPage(response.headers.link); | |||
| if(nextUrl) { | |||
| if(nextUrl && !manualPagination) { | |||
| if (!options) { | |||
| options = {}; | |||
| } | |||
| options.page = parseInt( | |||
There was a problem hiding this comment.
If someone can clarify this, I would appreciate it.
Why does the page number need to be manually extracted and appended to the options?
nextUrl is being extracted from the headers, wouldn't it have all the options it already needs? Then by calling this._requestAllPages(nextUrl, options, cb, results); the options are being re-added to the url, which results in duplicate parameters in the path, eg:
/search/repositories?q=tetris+language%3Aassembly&sort=stars&order=desc&type=all&per_page=100&page=4&q=tetris+language:assembly&sort=stars&order=desc&type=all&per_page=100&page=4
Although this might just be a workaround for other endpoints, since I have't done exploration into how other endpoints paginate.
Changes:
Github.searchfunction.pageoption is passed into_requestAllPages, it will only fetch that single page, instead of attempting to fetch all of the pages.resultsparameter from_requestAllPages, since that is only meant to be used internally anyway. It's trivial to re-introduce, so let me know if this makes sense to do, since this could break some backwards compatibility if folks are using an unsupported argument.rawHeaderinstead ofheader, since the tests were not paginating. This was introduced when the spec for fixture files changed from nock@7 to nock@8.Let me know if this is the right approach. I've only run tests for the search endpoints (due to lack of access to the testing repo), so I'm not sure if this is going to introduce errors elsewhere.
Closes #406
This could be a potential solution for #460, although introducing a
limitcould be an alternate solution.