octokit / rest.js

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

TypeScript signature of async iterators changed unexpectedly: #82

Open bcoe opened 3 years ago

bcoe commented 3 years ago

Checklist

Environment

Versions

├─┬ @octokit/plugin-enterprise-compatibility@1.3.0
│ ├─┬ @octokit/request-error@2.1.0
│ └─┬ @octokit/types@6.18.1
│   └── @octokit/openapi-types@8.2.1
├─┬ @octokit/plugin-paginate-rest@2.13.5
├─┬ @octokit/rest@18.6.7
│ ├─┬ @octokit/core@3.5.1
│ │ ├─┬ @octokit/graphql@4.6.4
│ │ ├─┬ @octokit/request@5.6.0
│ │ │ ├─┬ @octokit/endpoint@6.0.12
│ ├── @octokit/plugin-request-log@1.0.4
│ └─┬ @octokit/plugin-rest-endpoint-methods@5.4.1
├─┬ @probot/octokit-plugin-config@1.1.0
├─┬ octokit-auth-probot@1.2.6
│ ├─┬ @octokit/auth-app@3.5.3
│ │ ├─┬ @octokit/auth-oauth-app@4.3.0
│ │ │ ├─┬ @octokit/auth-oauth-device@3.1.2
│ │ ├─┬ @octokit/auth-oauth-user@1.3.0
│ │ │ ├─┬ @octokit/oauth-methods@1.2.4
│ │ │ │ ├── @octokit/oauth-authorization-url@4.3.2
│ ├─┬ @octokit/auth-token@2.4.5
│ ├─┬ @octokit/auth-unauthenticated@2.1.0
│ ├─┬ @octokit/plugin-retry@3.0.9
│ ├─┬ @octokit/plugin-throttling@3.5.1
│ ├─┬ @octokit/webhooks@7.21.0

What happened?

In a minor version of one of the octokit dependencies, the method signature of pagination has changed significantly. This was the old code that worked:

    const installationRepositoriesPaginated = octokit.paginate.iterator(
      octokit.apps.listReposAccessibleToInstallation,
      {
        mediaType: {
          previews: ['machine-man'],
        },
      }
    );
    for await (const response of installationRepositoriesPaginated) {
      for (const repo of response.data) {
        yield repo;
      }
    }

When updating to the latest versions of libraries in package-lock.json, compilation fails due to a significantly different type:

src/gcf-utils.ts:651:26 - error TS2488: Type '{ total_count: number; repositories: { id: number; node_id: string; name: string; full_name: string; license: { key: string; name: string; url: string | null; spdx_id: string | null; node_id: string; html_url?: string | undefined; } | null; ... 82 more ...; starred_at?: string | undefined; }[]; repository_selection?...' must have a '[Symbol.iterator]()' method that returns an iterator.

Rather than being an array, data now returns an object with a top level repositories field which is an array.

Minimal test case to reproduce the problem

Renovates routine lock maintenance surfaces issue:

https://github.com/googleapis/repo-automation-bots/pull/2241

What did you expect to happen?

Type to remain similar in a minor or patch update.

What the problem might be

Not sure.

gr2m commented 3 years ago

If you need to iterate through repositories I'd recommend using https://github.com/octokit/app.js/ :) I hope to migrate Probot to it with the next version, but not sure how long that will take.

I'm looking into the type change now

gr2m commented 3 years ago

Hm it looks like it returns both, which is incorrect. But when you do the for ... of loop it gets the types from the array as it should. So far I cannot reproduce the problem locally, but I'm still looking into it.

gr2m commented 3 years ago

I couldn't reproduce the problem with

mkdir test
cd test
npm init -y
npm i @octokit/rest

Then create index.ts

import { Octokit } from "@octokit/rest";
const octokit = new Octokit();

export async function* test() {
  const installationRepositoriesPaginated = octokit.paginate.iterator(
    octokit.apps.listReposAccessibleToInstallation,
    {
      mediaType: {
        previews: ["machine-man"],
      },
    }
  );

  for await (const response of installationRepositoriesPaginated) {
    for (const repo of response.data) {
      yield repo;
    }
  }
}

And I don't see any relevant diffs from my dependency trees to the one you shared. Interestingly, both @octokit/types and @octokit/openapi-types are missing in your dependency tree, can you think of why?

bcoe commented 3 years ago

@gr2m I did a bit more experimentation and the issue crops up for us between @octokit/rest@18.6.3 and @octokit/rest@18.6.4, if I downgrade to 18.6.3 it compiles, if I upgrade to 18.6.4 it fails.

gr2m commented 3 years ago

Still no luck on my side reproducing the problem, sorry 😞 Could you create a repository with my test code above that throws the error when building?

gr2m commented 3 years ago

I think it has something todo with the configuration in gts/tsconfig-google.json. I can reproduce the build error with this config

{
  "extends": "./node_modules/gts/tsconfig-google.json",
  "compilerOptions": {
    "lib": ["es2018", "dom"],
    "esModuleInterop": true,
    "rootDir": ".",
    "outDir": "build"
  },
  "include": ["*.ts"]
}

The error does not occur if I remove the extends key

gr2m commented 3 years ago

It's the compilerOptions.strict setting

{
  "compilerOptions": {
    "strict": true,
    "lib": ["es2018"]
  },
  "include": ["*.ts"]
}
gr2m commented 3 years ago

I'll have to get back to this later. We do have the strict setting in https://github.com/octokit/plugin-paginate-rest.js/blob/master/tsconfig.json. I think the next step would be is to add your code as a failing test to https://github.com/octokit/plugin-paginate-rest.js. I have to get back to this next week. If you have time and could add the failing test, that'd be very helpful in moving this forward

bcoe commented 3 years ago

@gr2m thanks for digging into this 👏 I'll play myself with settings.

bcoe commented 3 years ago

I was able to get things compiling by merging strict false into the config, this didn't seem to cause any major issues. Consider this a low priority problem for us currently, thank you for helping figure this out.

honnix commented 3 years ago

FWIW, we are having the same problem.

bcomnes commented 1 year ago

Seeing something similar recently as well.

EDIT: oopps didn't see the date. I'll post more if I can figure out what caused this.