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

Derive getUserOctokit return type from Octokit option #262

Open
inga-lovinde opened this issue Aug 10, 2021 · 2 comments
Open

Derive getUserOctokit return type from Octokit option #262

inga-lovinde opened this issue Aug 10, 2021 · 2 comments
Labels
Status: Up for grabs Issues that are ready to be worked on by anyone Type: Bug Something isn't working as documented typescript Relevant to TypeScript users only
Projects

Comments

@inga-lovinde
Copy link

inga-lovinde commented Aug 10, 2021

What happened?

import { Octokit, OAuthApp } from 'octokit'

const app = new OAuthApp({
  clientType: 'oauth-app',
  clientId: 'fakeClientId',
  clientSecret: 'fakeClientSecret',
})

const octokit: Octokit = await app.getUserOctokit({ code: 'fakeCode' })

causes a type error in the last line, because the unwrapped return value of app.getUserOctokit is not assignable to the constant of type Octokit.

What did you expect to happen?

No type errors.

What the problem might be

app.getUserOctokit returns a promise of an instance type of Octokit option.

However, app.getUserOctokit is declared as returning Promise<OctokitInstance>, where OctokitInstance is hardcoded to be an instance type of OAuthTypeOctokit, regardless of the options passed to OAuthApp constructor.

It should probably be declared as returning Promise<OctokitTypeFromOptions<TOptions>> instead.

A similar issue (but about octokit field instead of getUserOctokit method) is #212.

@inga-lovinde inga-lovinde added the Type: Bug Something isn't working as documented label Aug 10, 2021
@ghost ghost added this to Bugs in JS Aug 10, 2021
@gr2m gr2m added the typescript Relevant to TypeScript users only label Aug 10, 2021
@gr2m
Copy link
Contributor

gr2m commented Aug 10, 2021

I was able to reproduce the issue. It might be further complicated by the fact that the Octokit constructor option is set via .defaults()

https://github.com/octokit/octokit.js/blob/e19f0cedb664746b9b7a5059a69488a5671aa4d0/src/app.ts#L9

would you like to give it a go and resolve the problem? I'd be happy to review PRs, but I won't be able to work on it myself anytime soon

@inga-lovinde
Copy link
Author

Unfortunately I don't think I will be able to prepare a full pull request.

However, the code changes should be quite easy, thanks to the work done in #212 . And the fact that the Octokit constructor option is set via .defaults shouldn't complicate things, because it was already handled in #212.

I would think that it is enough to redeclare GetUserOctokitWithStateInterface as

export interface GetUserOctokitWithStateInterface<
  TClientType extends ClientType,
  TOctokitInstance extends OctokitInstance
> {
  (
    options: TClientType extends "oauth-app"
      ? GetUserOctokitOAuthAppOptions
      : GetUserOctokitGitHubAppOptions
  ): Promise<TOctokitInstance>;
}

and to redeclare getUserOctokit as

  getUserOctokit: GetUserOctokitWithStateInterface<
    ClientTypeFromOptions<TOptions>,
    OctokitTypeFromOptions<TOptions>
  >;

@gr2m gr2m added the Status: Up for grabs Issues that are ready to be worked on by anyone label Aug 10, 2021
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 typescript Relevant to TypeScript users only
Projects
No open projects
JS
  
Bugs
Development

No branches or pull requests

2 participants