octokit / octokit.js

The all-batteries-included GitHub SDK for Browsers, Node.js, and Deno.
MIT License
6.86k stars 1k forks source link

[BUG]: Typings not resolving correctly for rest/checks/get & rest/checks/update #2564

Closed Codex- closed 8 months ago

Codex- commented 9 months ago

What happened?

I noticed this when trying to use octokit.rest.checks.update:

const octokit = github.getOctokit(config.token);

// Should error with the required props not being provided
octokit.rest.checks.update({});

I inspected the params further, comparing with create, using status as an example:

// "queued" | "in_progress" | "completed" | undefined
type CheckCreateStatus = NonNullable<Parameters<Octokit["rest"]["checks"]["create"]>[0]>["status"];

// unknown
type CheckUpdateStatus = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["status"];

Appears to be related to the type generation for /repos/{owner}/{repo}/check-runs/{check_run_id}

The documentation indicates that status should be available, but trying to get any types for this endpoint results in unknown:

// unknown
type owner = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["owner"];
// unknown
type repo = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["repo"];
// unknown
type name = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["name"];
// unknown
type details_url = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["details_url"];
// unknown
type external_id = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["external_id"];
// unknown
type started_at = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["started_at"];
// unknown
type status = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["status"];
// unknown
type conclusion = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["conclusion"];
// unknown
type completed_at = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["completed_at"];
// unknown
type output = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["output"];
// unknown
type actions = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["actions"];

Versions

Node 20.8.1

  /@actions/core@1.10.1:
    resolution: {integrity: sha512-3lBR9EDAY+iYIpTnTIXmWcNbX3T2kCkAEQGIQx4NVQ0575nk2k3GRZDTPQG+vVtS2izSLmINlxXf0uLtnrTP+g==}
    dependencies:
      '@actions/http-client': 2.2.0
      uuid: 8.3.2
    dev: false

  /@actions/github@6.0.0:
    resolution: {integrity: sha512-alScpSVnYmjNEXboZjarjukQEzgCRmjMv6Xj47fsdnqGS73bjJNDpiiXmp8jr0UZLdUB6d9jW63IcmddUP+l0g==}
    dependencies:
      '@actions/http-client': 2.2.0
      '@octokit/core': 5.0.1
      '@octokit/plugin-paginate-rest': 9.1.0(@octokit/core@5.0.1)
      '@octokit/plugin-rest-endpoint-methods': 10.1.0(@octokit/core@5.0.1)
    dev: false

Relevant log output

No response

Code of Conduct

github-actions[bot] commented 9 months ago

👋 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 labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

wolfy1339 commented 9 months ago

I just double checked the generated endpoints and types, and the URL parameters are set as required, the body parameters are all defined as you would expect.

Are you able to use Octokit directly instead of using @actions/github in order to isolate the issue?

Codex- commented 8 months ago

The same results for me when using the latest octokit directly.

import type { Octokit } from "octokit";

// unknown
type owner = NonNullable<Parameters<Octokit["rest"]["checks"]["update"]>[0]>["owner"];

// Same results for the other expected types too

update: image

create: image

wolfy1339 commented 8 months ago

Thanks for confirming that. I can't seem to reproduce this myself.

In the Typescript transpilation of the OpenAPI spec everything seems fine.

Are you able to diagnose this?

wolfy1339 commented 8 months ago

The base types from @octokit/types are correct, it's when we create method types over in @octokit/plugin-rest-endpoint-methods where things start going foul.

You can see in my TypeScript playground the issue here.

The types for this endpoint in @octokit/plugin-rest-endpoint-methods:

https://github.com/octokit/plugin-rest-endpoint-methods.js/blob/51ea414f2401f1725e353f7a33288a4fb1faf873/src/generated/parameters-and-response-types.ts#L1694-L1698

I have narrowed it down to the Omit<[...], "baseUrl" | "headers" | "mediaType"> It seems that TypeScript narrows it down to { [key: string]: unknown }

@gr2m Do you recall why the Omit is there? The earliest that I can find it's usage is from https://github.com/octokit/plugin-rest-endpoint-methods.js/commit/7ed9fc41b3c714e851610111ae785841df0f77fd

gr2m commented 8 months ago

Do you recall why the Omit is there

I don't, sorry 😞 "baseUrl" | "headers" | "mediaType" are all generic request options, not specific to any rest API endpoint, I assume they are added back at a later point, so that the parameters types are only containing types for the respective endpoint, without the generic ones

wolfy1339 commented 8 months ago

At that point there where the Omit is currently, there doesn't seem to be any request related parameters. It's only the endpoint parameters themselves.

gr2m commented 8 months ago

feel free to experiment and remove it, I was learning TypeScript as I was building this, it's likely that I messed things up. The types I built for https://github.com/octokit/octokit-next.js are more elegant 🤷 I really wish we could complete that work

wolfy1339 commented 8 months ago

I removed the Omit from the types, and ran the tests for @octokit/rest along with the tests for octokit with the updated types and every test passes, including the typescript test.

I believe that that would alleviate this issue. I will prepare a PR to fix that

wolfy1339 commented 8 months ago

It seems we have run into this issue before: https://github.com/octokit/plugin-rest-endpoint-methods.js/issues/441