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

[BUG]: Regression with 11.4.2 adding & OctokitResponse<...> to return type #661

Open
1 task done
chriscarpenter12 opened this issue Feb 18, 2025 · 4 comments · May be fixed by #662
Open
1 task done

[BUG]: Regression with 11.4.2 adding & OctokitResponse<...> to return type #661

chriscarpenter12 opened this issue Feb 18, 2025 · 4 comments · May be fixed by #662
Labels
Type: Bug Something isn't working as documented

Comments

@chriscarpenter12
Copy link

What happened?

From 11.4.1 to 11.4.2 we're seeing an innocuous regression from PR #653

We have a couple instances like this that are showing a return type with a union of & OctokitResponse<...> to the expected data response type. Assuming from here.

Example code that's pulling org packages from GET /orgs/{org}/packages/{package_type}/{package_name}/versions

import { components } from '@octokit/openapi-types';

type Package = components['schemas']['package-version'];

  private async getPackages(): Promise<Package[]> {
    let packages = await this.octokit.paginate(this.octokit.rest.packages.getAllPackageVersionsForPackageOwnedByOrg, {
      org: this.flags.org,
      package_name: this.flags.name,
      package_type: this.flags.type,
      per_page: 100,
    });

    console.log(util.inspect(packages, false, null, true));

    if (this.flags?.filter !== '') {
      const filterRegex = new RegExp(this.flags.filter);
      packages = packages.filter((pkg) => filterRegex.test(pkg.name));
    }

    return packages;
  }

Before 11.4.2 this wasn't included:

Image

Type '{ id: number; name: string; url: string; package_html_url: string; html_url?: string | undefined; license?: string | undefined; description?: string | undefined; created_at: string; updated_at: string; deleted_at?: string | undefined; metadata?: { ...; } | undefined; }[]' is missing the following properties from type 'OctokitResponse<{ id: number; name: string; url: string; package_html_url: string; html_url?: string | undefined; license?: string | undefined; description?: string | undefined; created_at: string; updated_at: string; deleted_at?: string | undefined; metadata?: { ...; } | undefined; }[], 200>': headers, status, url, data

And looking at the actual data returned, I don't see any additional fields that aren't in the opanapi type.

[
  {
    id: 00000,
    name: '1.0.2-prettier-3-5-1.2',
    url: 'https://api.github.com/orgs/XXXX/packages/npm/playwright-testing/versions/00000',
    package_html_url: 'https://github.com/orgs/XXXX/packages/npm/package/playwright-testing',
    created_at: '2025-02-17T16:56:07Z',
    updated_at: '2025-02-17T16:56:07Z',
    html_url: 'https://github.com/orgs/XXXX/packages/npm/playwright-testing/00000',
    metadata: { package_type: 'npm' }
  },
  ...
]

Versions

Node v20.15.0
octokit 4.1.1
@octokit/plugin-paginate-rest 11.4.2
@octokit/types 13.7.0

Relevant log output

Code of Conduct

  • I agree to follow this project's Code of Conduct
@chriscarpenter12 chriscarpenter12 added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Feb 18, 2025
Copy link
Contributor

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@wolfy1339
Copy link
Member

wolfy1339 commented Feb 18, 2025

The issue was that I was trying to fix #652, which itself was a regression from #637.

At the root, I tried to fix the issue of Typescript not correctly finding the data and returning bad types.
#350

What I'm trying to do is follow the JavaScript code, return the pagination data without the pagination and add the keys to the data.

https://github.com/octokit/plugin-paginate-rest.js/blob/main/src%2Fnormalize-paginated-list-response.ts#L47-L55

@wolfy1339
Copy link
Member

Also, another fyi, it's bad practice to depend on @octokit/openapi-types directly as the version can get out of sync with the Octokit packages and cause type issues due to updated response types.

For this package you can use the following:

import type { PaginatingEndpoints } from "@octokit/plugin-paginate-rest";

type Package = PaginatingEndpoints["GET /orgs/{org}/packages/{package_type}/{package_name}/versions"]["response"]["data"][0];

@wolfy1339
Copy link
Member

I've found the root of the issue,

type GetPaginationKeys<T> = T extends { data: any[] }
? T
: T extends { data: object }
? Pick<
T["data"],
Extract<
keyof T["data"],
"repository_selection" | "total_count" | "incomplete_results"
>
>
: never;

We are returning the full OctokitResponse in there, when we should instead probably be returning an empty object

wolfy1339 added a commit that referenced this issue Feb 18, 2025
Instead of returning `T` in `GetPaginationKeys<T>`, we return an empty object, because T would be an `OctokitResponse<T, S>`, and there is no response data in the `data` field

Fixes #661
@wolfy1339 wolfy1339 removed the Status: Triage This is being looked at and prioritized label Feb 18, 2025
@wolfy1339 wolfy1339 moved this from 🆕 Triage to 🏗 In progress in 🧰 Octokit Active Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
Status: 🏗 In progress
2 participants