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

Add pagination to search endpoints #583

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/GitHub.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if jsdoc understands how to reach into other files (Search.js). Let me know if this will be a problem.

* @return {Search}
*/
search(query) {
return new Search(query, this.__auth, this.__apiBase);
search(searchParameters) {
return new Search(searchParameters, this.__auth, this.__apiBase);
}

/**
Expand Down
35 changes: 26 additions & 9 deletions lib/Requestable.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,13 +235,30 @@ class Requestable {
* @param {string} path - the path to request
* @param {Object} options - the query parameters to include
* @param {Requestable.callback} [cb] - the function to receive the data. The returned data will always be an array.
* @param {Object[]} results - the partial results. This argument is intended for internal use only.
* @return {Promise} - a promise which will resolve when all pages have been fetched
* @deprecated This will be folded into {@link Requestable#_request} in the 2.0 release.
*/
_requestAllPages(path, options, cb, results) {
results = results || [];
_requestAllPages(path, options, cb) {
let manualPagination = false;
if (typeof options.page !== 'undefined') {
manualPagination = true;
}

return this._requestAllPagesHelper(path, options, cb, [], manualPagination);
}

/**
* Perform the logic of fetching multiple pages
* @private
* @param {string} path - the path to request
* @param {Object} options - the query parameters to include
* @param {Requestable.callback} [cb] - the function to receive the data. The returned data will always be an array.
* @param {Object[]} results - the partial results. This argument is intended for internal use only.
* @param {boolean} manualPagination - the flag to decide if multiple pages should be fetched
* @return {Promise} - a promise which will resolve when all pages have been fetched
* @deprecated This will be folded into {@link Requestable#_request} in the 2.0 release.
*/
_requestAllPagesHelper(path, options, cb, results, manualPagination) {
return this._request('GET', path, options)
.then((response) => {
let thisGroup;
Expand All @@ -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(
Copy link
Author

@alaycock alaycock Sep 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nextUrl.match(/([&\?]page=[0-9]*)/g)
.shift()
.split('=')
.pop()
nextUrl.match(/([&\?]page=[0-9]*)/g)
.shift()
.split('=')
.pop()
);
if (!(options && typeof options.page !== 'number')) {
log(`getting next page: ${nextUrl}`);
return this._requestAllPages(nextUrl, options, cb, results);
return this._requestAllPagesHelper(nextUrl, options, cb, results, false);
}
}

Expand Down
2 changes: 2 additions & 0 deletions lib/Search.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ class Search extends Requestable {
* @param {string} sort - the sort field, one of `stars`, `forks`, or `updated`.
* Default is [best match](https://developer.github.com/v3/search/#ranking-search-results)
* @param {string} order - the ordering, either `asc` or `desc`
* @param {number} page - the page number to fetch, for manual pagination
* @param {number} per_page - the number of results to fetch per page, for manual pagination
*/
/**
* Perform a search on the GitHub API
Expand Down
Loading