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

TypeScript types appear to be incorrect for paginate() on apps.listReposAccessibleToInstallation #350

Open
reececomo opened this issue Aug 24, 2021 · 12 comments
Labels
Status: Needs info Full requirements are not yet known, so implementation should not be started Type: Bug Something isn't working as documented
Projects

Comments

@reececomo
Copy link

Hey there.

When running await paginate(...) on apps.listReposAccessibleToInstallation, the TypeScript inferred data type is not correct.

  // context is an Octokit with an installation.id

  const repos = await context.paginate(context.apps.listReposAccessibleToInstallation, {
    per_page: 100,
  });

  // TypeScript compiler/VSCode sees `repos` as:
  // {
  //   total_count: number,
  //   repositories: { id: number,  /*...*/ }[ ],
  //   repository_selection: ?string
  // }

But really at runtime it contains repositories data array:

  console.log(repos)
  // [
  //   { id: 123,  /*...*/ },
  //   { id: 456,  /*...*/ }
  // ]

Workaround

  // FIXME: Have been doing this as a workaround.
  const installationRepos = repos.repositories ?? repos;

  installationRepos.forEach(/*...*/);
@ghost ghost added this to Inbox in JS Aug 24, 2021
@gr2m gr2m added the Type: Support Any questions, information, or general needs around the SDK or GitHub APIs label Aug 24, 2021
@gr2m gr2m self-assigned this Aug 24, 2021
@ghost ghost moved this from Inbox to In progress in JS Aug 24, 2021
@gr2m
Copy link
Contributor

gr2m commented Aug 24, 2021

Hmm I have a test for this exact endpoint and TS compiles without error:

// https://developer.github.com/v3/apps/installations/#list-repositories
export async function knownRouteWithNamespacedResponse() {
const results = await octokit.paginate("GET /installation/repositories");
for (const result of results) {
console.log(result.owner.login);
}
}
export async function knownRouteWithNamespacedResponseIterator() {
for await (const response of octokit.paginate.iterator(
"GET /installation/repositories"
)) {
console.log(response.data[0].owner.login);
}
}

Might this be a problem with a different TS config or TS version?

Note that we do create a union between the actual endpoint types which is { total_count, repositories, ... } and our normalized format that we do within @octokit/plugin-paginate-rest which is { ...repositoryProperties}[]. We do that because some of the returned properties from the API cannot be derived from another place, so you basically get an array with custom properties such as .total_count

@gr2m gr2m added the Status: Needs info Full requirements are not yet known, so implementation should not be started label Aug 24, 2021
@ghost ghost moved this from In progress to Awaiting response in JS Aug 24, 2021
@gr2m
Copy link
Contributor

gr2m commented Sep 12, 2021

closing due to inactivity

@gr2m gr2m closed this as completed Sep 12, 2021
JS automation moved this from Awaiting response to Done Sep 12, 2021
@reececomo
Copy link
Author

Might this be a problem with a different TS config or TS version?

Possibly! I believe I was on latest, not sure if VSCode pulls in a different bin though 👍

Thanks for having a look @gr2m

@nodify-at
Copy link

Hi,

we have exact same issue.

Code:

      repos = await this.appInstallsClient.paginate(
        this.appInstallsClient.apps.listReposAccessibleToInstallation,
        {
          per_page: 100,
          headers: {
            'If-None-Match': this.reposResponse?.headers.etag,
          },
        },
        response =>
          response.data.filter(repository => isActiveRepository(repository)),
      );

Typescript expects an object ( { repositories, total_count, ..} ).
Typescript Version: ^4.4.4

@JamieMagee
Copy link

I am also seeing this issue. @gr2m can you reopen please?

@gr2m
Copy link
Contributor

gr2m commented Nov 2, 2021

Note that @octokit is currently unmaintained, subscribe to octokit/octokit.js#620 (comment) for updates

@gr2m gr2m reopened this Nov 2, 2021
@Shegox
Copy link

Shegox commented Apr 1, 2022

I saw the same problem today, after updating to probot@12.2.2 and typescript@4.6.3. Typescript complained about that it's no longer an array with .length attribute, but rather the object.

Anyway @reececomo thanks a lot for providing a workaround. That works flawless 👍

@reececomo
Copy link
Author

No problem @Shegox! Sorry it has to be this way 😓

@wolfy1339
Copy link
Member

wolfy1339 commented May 19, 2023

I can confirm that this is happening to me locally when cloning this repo in VSCode with TypeScript 5.0.4, but not when running the test:ts script with the same TypeScript version.

The type of the returned value for the pagination is not an array.

/**
* Paginate a request using an endpoint method and parameters
*
* @param {string} request Request method (`octokit.request` or `@octokit/request`)
* @param {object} parameters? URL, query or body parameters, as well as `headers`, `mediaType.format`, `request`, or `baseUrl`.
*/
<R extends OctokitTypes.RequestInterface>(
request: R,
parameters?: Parameters<R>[0]
): Promise<
NormalizeResponse<OctokitTypes.GetResponseTypeFromEndpointMethod<R>>["data"]
>;

Maybe the returned type should be Promise<NormalizeResponse<OctokitTypes.GetResponseTypeFromEndpointMethod<R>>["data"][]>;

@wolfy1339 wolfy1339 added Type: Bug Something isn't working as documented and removed Type: Support Any questions, information, or general needs around the SDK or GitHub APIs labels May 19, 2023
@wolfy1339 wolfy1339 moved this from Done to Bugs in JS May 19, 2023
@ghost ghost moved this from Bugs to In progress in JS May 19, 2023
@martincostello
Copy link

I've stumbled into this issue as well. If it helps anyone else until this is resolved, I'm doing this as a workaround:

// N.B. I've only included the properties I happen to use here.
type InstallationRepository = {
  name: string;
  full_name: string;
  owner: {
    login: string;
  };
  default_branch: string;
  archived: boolean;
  fork: boolean;
  html_url: string;
  is_template: boolean;
  topics: string[];
};

const repositories = (await octokit.paginate(
  octokit.rest.apps.listReposAccessibleToInstallation,
  { per_page: 100 }
)) as unknown as InstallationRepository[];

@Siumauricio
Copy link

I have the same issue i did this to solve

      const repositories = (await octokit.paginate(
        octokit.rest.apps.listReposAccessibleToInstallation,
      )) as unknown as Awaited<
        ReturnType<typeof octokit.rest.apps.listReposAccessibleToInstallation>
      >["data"]["repositories"];

@gr2m gr2m removed their assignment Feb 5, 2024
@kamilogorek
Copy link

For completeness, other than listReposAccessibleToInstallation, another method that is affected by this issue is listInstallationsForAuthenticatedUser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs info Full requirements are not yet known, so implementation should not be started Type: Bug Something isn't working as documented
Projects
Status: 🔥 Backlog
JS
  
In progress
Development

No branches or pull requests

9 participants