Update types to allow either app or user auth #113
Conversation
| consumer_key?: never; | ||
| /** consumer secret from Twitter */ | ||
| consumer_secret?: never; | ||
| /** access token key from your User (oauth_token) */ | ||
| access_token_key?: never; | ||
| /** access token secret from your User (oauth_token_secret) */ | ||
| access_token_secret?: never; |
There was a problem hiding this comment.
defining all the keys as "never" seems a bit useless to me. If we were to define the TwitterOptionsAppAuthentication and TwitterOptionsUserAuthentication as types and not interfaces, we could just leave out anything that's not supposed to be included.
There was a problem hiding this comment.
Thank's @T0TProduction for your feedback.
The problem is that unions in TypeScript are not exclusive (see microsoft/TypeScript#14094).
This means that if we leave out the nevers, an object with a bearer_token and a consumer_key would be a valid TwitterOptions object.
This StackOverflow comment sums the problem up: https://stackoverflow.com/a/61281828
I tried to use types instead of interfaces as you described it, but the problem remained the same.
In my opinion, we should use never to have stricter rules.
There was a problem hiding this comment.
Ah, I see!
I agree, if we do not want to allow both at the same time, that's the best (and only) option.
I just checked the code, and now I'm a bit confused though:
according to my understanding of the constructor, it could actually handle both options ( but set the type to 'App' none the less )
Maybe it should ignore keys after already having a token; or throw warnings/errors when multiple credentials are added simultaneously.
Then the types/interfaces should be exactly as you said, to support that choice of exclusivity
| @@ -131,7 +136,20 @@ interface TwitterOptions { | |||
| /** access token secret from your User (oauth_token_secret) */ | |||
| access_token_secret?: OauthTokenSecret; | |||
| /** bearer token */ | |||
| bearer_token?: string; | |||
| bearer_token?: never; | |||
There was a problem hiding this comment.
same as mentioned in the other comment, is never really needed?
Initializing a new client with App authentication creates an TypeScript error:
With this PR
TwitterOptionsaccepts either the fields for User authentication (consumer_key,consumer_secret,access_token_key,access_token_secret) or for App authentication (bearer_token) as described in the README.