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.

Sign up for GitHub

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

Update types to allow either app or user auth #113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update types to allow either app or user auth #113

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,16 @@ export default class Twitter {
Currently Twitter themselves convert it to strings for the API though, so this change will come some time in the far future. */
type snowflake = string;

interface TwitterOptions {
type TwitterOptions = TwitterOptionsUserAuthentication | TwitterOptionsAppAuthentication

interface TwitterOptionsBasics {
/** "api" is the default (change for other subdomains) */
subdomain?: string;
/** version "1.1" is the default (change for other subdomains) */
version?: string;
}

interface TwitterOptionsUserAuthentication extends TwitterOptionsBasics {
/** consumer key from Twitter. */
consumer_key: string;
/** consumer secret from Twitter */
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

same as mentioned in the other comment, is never really needed?

}

interface TwitterOptionsAppAuthentication extends TwitterOptionsBasics {
/** consumer key from Twitter. */
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;
Comment on lines +144 to +150
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

/** bearer token */
bearer_token: string;
}

type OauthToken = string;
Expand Down