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 signature of async iterators changed unexpectedly: #82

Open
3 tasks done
bcoe opened this issue Jul 7, 2021 · 12 comments
Open
3 tasks done

TypeScript signature of async iterators changed unexpectedly: #82

bcoe opened this issue Jul 7, 2021 · 12 comments
Labels
Type: Support Any questions, information, or general needs around the SDK or GitHub APIs typescript
Projects

Comments

@bcoe
Copy link

bcoe commented Jul 7, 2021

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:

googleapis/repo-automation-bots#2241

What did you expect to happen?

Type to remain similar in a minor or patch update.

What the problem might be

Not sure.

@bcoe bcoe added the Type: Bug Something isn't working as documented label Jul 7, 2021
@ghost ghost added this to Bugs in JS Jul 7, 2021
@gr2m
Copy link
Contributor

gr2m commented Jul 8, 2021

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 gr2m self-assigned this Jul 8, 2021
@ghost ghost moved this from Bugs to In progress in JS Jul 8, 2021
@gr2m
Copy link
Contributor

gr2m commented Jul 8, 2021

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
Copy link
Contributor

gr2m commented Jul 8, 2021

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?

@gr2m gr2m added Type: Support Any questions, information, or general needs around the SDK or GitHub APIs and removed Type: Bug Something isn't working as documented labels Jul 8, 2021
@bcoe
Copy link
Author

bcoe commented Jul 9, 2021

@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
Copy link
Contributor

gr2m commented Jul 9, 2021

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
Copy link
Contributor

gr2m commented Jul 9, 2021

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
Copy link
Contributor

gr2m commented Jul 9, 2021

It's the compilerOptions.strict setting

{
  "compilerOptions": {
    "strict": true,
    "lib": ["es2018"]
  },
  "include": ["*.ts"]
}

@gr2m
Copy link
Contributor

gr2m commented Jul 9, 2021

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
Copy link
Author

bcoe commented Jul 9, 2021

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

@bcoe
Copy link
Author

bcoe commented Jul 9, 2021

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.

@gr2m gr2m removed their assignment Aug 12, 2021
@ghost ghost moved this from In progress to Support in JS Aug 12, 2021
@honnix
Copy link

honnix commented Aug 16, 2021

FWIW, we are having the same problem.

@bcomnes
Copy link

bcomnes commented Jul 6, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Support Any questions, information, or general needs around the SDK or GitHub APIs typescript
Projects
No open projects
JS
  
Support
Development

No branches or pull requests

5 participants