Skip to content

Accept Octokit.Options in the GitHub constructor#113

Merged
jclem merged 3 commits intomasterfrom
client-options
Sep 5, 2019
Merged

Accept Octokit.Options in the GitHub constructor#113
jclem merged 3 commits intomasterfrom
client-options

Conversation

@jclem
Copy link
Copy Markdown
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

constructor(token: string) {
super({auth: `token ${token}`})
constructor(token: string, opts: Omit<Octokit.Options, 'auth'> = {}) {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

damccorm commented Sep 5, 2019

@jclem
Copy link
Copy Markdown
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
Copy Markdown
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