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]: Typings not resolving correctly for rest/checks/get & rest/checks/update #2564

Closed
1 task done
Codex- opened this issue Oct 25, 2023 · 10 comments · Fixed by octokit/plugin-rest-endpoint-methods.js#694
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented
Projects

Comments

@Codex-
Copy link

Codex- commented Oct 25, 2023

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

  • I agree to follow this project's Code of Conduct
@Codex- Codex- added Status: Triage This is being looked at and prioritized Type: Bug Something isn't working as documented labels Oct 25, 2023
@ghost ghost added this to Bugs in JS Oct 25, 2023
@github-actions
Copy link

👋 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! 🚀

@kfcampbell kfcampbell added Status: Up for grabs Issues that are ready to be worked on by anyone and removed Status: Triage This is being looked at and prioritized labels Oct 26, 2023
@wolfy1339
Copy link
Member

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

Codex- commented Oct 30, 2023

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

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

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 octokit/plugin-rest-endpoint-methods.js@7ed9fc4

@gr2m
Copy link
Contributor

gr2m commented Nov 9, 2023

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

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

gr2m commented Nov 9, 2023

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented
Projects
Archived in project
JS
  
Bugs
4 participants