Skip to content

Conversation

@kachick
Copy link
Contributor

@kachick kachick commented Jun 27, 2022

I have faced CI broken in Bump @octokit/types from 6.37.0 to 6.37.1 ref: https://github.com/kachick/wait-other-jobs/runs/7064440245?check_suite_focus=true.

It looks type mismatch with https://github.com/octokit/plugin-paginate-rest.js.
Removing

(route: Route, options?: RequestParameters): Promise<OctokitResponse<any>>;
passed typecheck, however I agree to revert as #404 at least in patch versions.

This PR is just a suggestion to includes the test. (Current test does not use exists tsconfig.json, so vscode is reporting some errors).

@levenleven I have cherry-picked your commit into this PR 🙏

@gr2m gr2m added Type: Bug Something isn't working as documented, or is being fixed typescript Relevant to TypeScript users only labels Jun 27, 2022
: RequestParameters
): R extends keyof Endpoints
? Promise<Endpoints[R]["response"]>
: Promise<OctokitResponse<any>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still need the overwrite? Shouldn't we combine this with L:26-29 as it was before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh...sorry, I have overlooked #404 is not reverting 🙇‍♂️
Personally I don't know any reason of keeping overloading, so just reverted in this PR.

5ef5f42
a1b9cd5

@gr2m
Copy link
Contributor

gr2m commented Jun 28, 2022

It's been a while but I think I found some smarter types when I worked on Octokit Next, maybe some of it might be applicable to the current types? Work on Ocotkit Next is stalled, unfortunately

https://github.com/octokit/octokit-next.js/blob/main/packages/types/request.d.ts#L30-L91

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@gr2m gr2m merged commit 11d6d8f into octokit:master Jun 28, 2022
@octokitbot
Copy link
Contributor

🎉 This PR is included in version 6.38.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kachick
Copy link
Contributor Author

kachick commented Jun 29, 2022

Thank you for your reviewing and merging! 🙏

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, or is being fixed typescript Relevant to TypeScript users only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants