octokit / rest.js

GitHub REST API client for JavaScript
https://octokit.github.io/rest.js
MIT License
545 stars 64 forks source link

`octokit.pulls.listRequestedReviewers` should not support pagination #33

Open nikclayton-dfinity opened 3 years ago

nikclayton-dfinity commented 3 years ago

Checklist

Environment

Versions

├── @octokit/webhooks@7.21.0

and:

├── @actions/github@4.0.0
├── @actions/core@1.2.6

What happened?

The GitHub documentation notes that listRequestedReviewers can be paginated.

I don't think it can -- the response object contains users and teams properties that are arrays that contain all the requested user and team reviewers within a single response, there's no pagination that needs to happen.

So if you write this:

const reviewers = octokit.paginate(octokit.pulls.listRequestedReviewers
  {...context.repo, pull_number})

then reviewers is actually a single element array, but the Tyepscript type inference system thinks it's an object.

So then you go and write:

for (const user of reviewers.users) {
    // ...
}

and you it compiles OK, but you get an error at runtime because the users property is not defined on the response, it's actually reviewers[0].users, but if you write reviewers[0].users in the code you get type errors.

What you actually need to write is:

const {data: reviewers} = await octokit.pulls.listRequestedReviewers(
  {...context.repo, pull_number})

i.e., remove the call to paginate and destructure the response.

I don't think there's much Octokit can do here in the code, but the documentation (i.e., https://octokit.github.io/rest.js/v18#pulls-list-requested-reviewers) could be updated to reference this and note that octokit.paginate should not be used with this API call.

gr2m commented 3 years ago

Thank you for reporting this problem! I created an issue with the docs team to find out if the endpoint does indeed not support any pagination. See https://github.com/github/docs/issues/2433

gr2m commented 3 years ago

It seems like it was filed internally, but the problem still seem to persist. I asked about the current status at https://github.com/github/docs/issues/2433

nitzanashi commented 2 years ago

Hey @gr2m is it possible that octokit.rest.issues.listMilestones endpoint is also not supported by paginate? I tried to use paginate for this endpoint

    const milestones = await octokit.paginate(octokit.rest.issues.listMilestones({
        owner,
        repo,
    }));

and got the same response as described in on top

gr2m commented 2 years ago

@nitzanashi your syntax is incorrect. You are passing the promise returned by octokit.rest.issues.listMilestones() to octokit.paginate(). Try this instead:

const milestones = await octokit.paginate(octokit.rest.issues.listMilestones,{
  owner,
  repo,
});

See my test case at https://runkit.com/gr2m/octokit-rest-js-33-comment/1.1.0

nitzanashi commented 2 years ago

@gr2m thanks a lot for the explanation!

gr2m commented 2 years ago

@nikclayton-dfinity just a quick update: an internal issue has been back in January but remains open as of today