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

return type of auth({ type: "installation", installationId, factory }) should be Promise<ReturnType<factory>> #204

Closed
gr2m opened this issue Oct 26, 2020 · 1 comment · Fixed by #301
Assignees
Labels
Type: Bug Something isn't working as documented, or is being fixed typescript Relevant to TypeScript users only
Projects

Comments

@gr2m
Copy link
Contributor

gr2m commented Oct 26, 2020

Follow up to https://github.com/octokit/auth-app.js/pull/198/files#diff-c54113cf61ec99691748a3890bfbeb00e10efb3f0a76f03a0fd9ec49072e410aR109-R111

Currently, when doing this:

const AppOctokit = Octokit.defaults({ authStrategy: createAppAuth, });
const appOctokit = new AppOctokit({
  auth: {
    id: APP_ID,
    privateKey: PRIVATE_KEY,
  },
});

const installationOctokit = await appOctokit.auth({
  type: "installation",
  installationId: INSTALLATION_ID,
  factory: (auth) => new appOctokit.constructor({ auth }),
});

The type for installationOctokit is unknown. What it should be though is the same as the type of appOctokit.

With the way the Types for authentication strategies are currently implemented, this change will be very hard to accomplish. Unfortunately it will be quite a rabbit hole, but I think it's about time we introduce a globally declared Octokit namespace on which we can define interfaces that plugins and users can extend.

I'd declare the global Octokit namespace in https://github.com/octokit/types.ts/

declare namespace Octokit {
    export interface AuthenticationStrategies {}
}

Then each Octokit authentication strategy module could extend the interface

declare namespace Octokit {
    export interface AuthenticationStrategies {
      OctokitApp: {
        authStrategy: Strategy;
        auth: AuthOptions;
      }
    }
}

We could still use OctokitTypes.StrategyInterface with implements to make sure the API surface for each authentication strategy is compatible.

See

@gr2m gr2m added Type: Bug Something isn't working as documented, or is being fixed typescript Relevant to TypeScript users only labels Oct 26, 2020
@ghost ghost added this to Bugs in JS Oct 26, 2020
@gr2m gr2m self-assigned this Apr 22, 2021
@ghost ghost moved this from Bugs to In progress in JS Apr 22, 2021
@gr2m gr2m closed this as completed in #301 Jun 25, 2021
JS automation moved this from In progress to Done Jun 25, 2021
@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 3.5.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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
1 participant