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

Accept Octokit.Options in the GitHub constructor #113

Merged
merged 3 commits into from
Sep 5, 2019
Merged

Conversation

jclem
Copy link
Contributor

@jclem jclem commented Sep 5, 2019

This adds an optional object of Octokit client options to the GitHub constructor. This allows for debugging, API previews, etc.

This PR also bumps TypeScript to 3.6.2 so that I can use the Omit type.

@jclem jclem requested a review from damccorm September 5, 2019 13:55
@@ -14,8 +14,8 @@ export class GitHub extends Octokit {
variables?: Variables
) => Promise<GraphQlQueryResponse>

constructor(token: string) {
super({auth: `token ${token}`})
constructor(token: string, opts: Omit<Octokit.Options, 'auth'> = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than omitting auth, can we just overload the constructor to something like:

constructor(token: string) { ... }
constructor(opts = {}) { ... }

That would also resolve #106

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but then how would we construct the authorization header for GraphQL requests?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

@damccorm
Copy link
Contributor

damccorm commented Sep 5, 2019

Might also be good to doc this as an option in https://github.com/actions/toolkit/blob/master/packages/github/README.md

@jclem
Copy link
Contributor Author

jclem commented Sep 5, 2019

The more I think about it, the more I like the simplicity of forcing a token to be provided and then accepting token-less options as a second argument. I think a token with no opts will be 99% of use.

For cases where more complex authorization is required (as is in the case of @josephperrott's issue), it seems just as easy to use @octokit/rest directly.

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I'm on board with that plan. LGTM (pending docs)

@jclem jclem merged commit 7772d5f into master Sep 5, 2019
@jclem jclem deleted the client-options branch September 5, 2019 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants