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

fix(RequestInterface): Revert method overloading with adding some tests #405

Merged
merged 6 commits into from Jun 28, 2022
Merged

fix(RequestInterface): Revert method overloading with adding some tests #405

merged 6 commits into from Jun 28, 2022

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 🙏

@ghost ghost added this to Inbox in JS Jun 27, 2022
@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
@ghost ghost moved this from Inbox to Bugs in JS 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
JS automation moved this from Bugs to Done Jun 28, 2022
@octokitbot
Copy link
Collaborator

🎉 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
No open projects
JS
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants