oauth-app.js icon indicating copy to clipboard operation
oauth-app.js copied to clipboard

Derive `getUserOctokit` return type from `Octokit` option

Open inga-lovinde opened this issue 2 years ago • 2 comments

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 avatar Aug 10 '21 14:08 inga-lovinde

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

gr2m avatar Aug 10 '21 16:08 gr2m

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>
  >;

inga-lovinde avatar Aug 10 '21 17:08 inga-lovinde