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

Throw error of unrecognized type in .auth({ type }) #216

Closed
gr2m opened this issue Nov 4, 2020 · 5 comments · Fixed by #249
Closed

Throw error of unrecognized type in .auth({ type }) #216

gr2m opened this issue Nov 4, 2020 · 5 comments · Fixed by #249
Assignees
Labels
Type: Feature New feature or request
Projects

Comments

@gr2m
Copy link
Contributor

gr2m commented Nov 4, 2020

I just spend a significant amount of time debugging a problem that was ultimately caused by a space after "installation ":

octokit.auth({ type: "installation " })

We should throw an error if a type is passed that is unknown. Otherwise it currently falls back to oauth, which is not what we want.

@gr2m gr2m added Type: Feature New feature or request help wanted labels Nov 4, 2020
@ghost ghost added this to Features in JS Nov 4, 2020
@oscard0m
Copy link
Member

Hi @gr2m

Taking a look into this.

Trying to reproduce it with TypeScript, the compiler complains about it:
https://codesandbox.io/s/octokit-auth-app-with-typo-lsg69

Do you have more details on how to reproduce this?

@gr2m
Copy link
Contributor Author

gr2m commented Jan 24, 2021

I'd like to throw an Error in the code, and not rely on the TypeScript definitions. hen using the auth strategy with Octokit, the octokit.auth() method does currently not inherit the types

@oscard0m
Copy link
Member

oscard0m commented Feb 2, 2021

Drafted this PR (#249) but at TS level does not have much sense since an invalid type is impossible to reach when using @octokit/auth-app package.


Checking at @octokit/core package, I found some TODOs which probably are the cause of the problem:

And checking at @octokit/types seems we are not exposing AuthTypes. What we would need is:

auth-app.js/src/types.ts

Lines 141 to 144 in f008655

export type AuthOptions = InstallationAuthOptions &
OAuthOptions & {
type: "app" | "installation" | "oauth";
};

Not sure how to proceed here:

  • To place AuthOptions in @octokit/types (and @octokit/auth-app and @octokit/core consume them from there) (?)
  • To add @octokit/auth-app as dependency for @octokit/core and consume AuthOptions directly (?)
  • Any other option I'm not considering (?)

@gr2m

@oscard0m oscard0m self-assigned this Feb 2, 2021
@gr2m
Copy link
Contributor Author

gr2m commented Feb 3, 2021

at TS level does not have much sense since an invalid type is impossible to reach when using @octokit/auth-app package.

That is a known limitation right now. See octokit/core.js#32 and https://github.com/octokit/rest.js/issues/1562#issuecomment-582678977. It comes back to the idea of having a global Octokit namespace that plugins can extend, it's a bigger project. I want to create a high level issue for this for further discussion. This will also address the ability to define custom event webhooks, for example

@gr2m gr2m closed this as completed in #249 Feb 3, 2021
JS automation moved this from Features to Done Feb 3, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2021

🎉 This issue has been resolved in version 2.11.0 🎉

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: Feature New feature or request
Projects
No open projects
JS
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants