Add typescript definition #49
Conversation
|
@kitten can you take a peek from TS good practices point of view when you have a moment. @kadikraman These seem legit to me, though I'd check if the optionality of fields is correct - IIRC the Only thing I'd change is the name |
index.d.ts
Outdated
|
|
||
| export function revoke( | ||
| properties: AppAuthInput, | ||
| { tokenToRevoke: string, sendClientId: boolean } |
There was a problem hiding this comment.
- The only properties needed for
revokeareclientIdandissuer. It doesn't matter if you pass in the whole configuration object, but it's not all used. I'm not sure if there's an easy way to reflect that in TypeScript? sendClientIdis optional, it defaults tofalse
There was a problem hiding this comment.
Thanks for the feedback!
As a proposal I made the clientId and issues in a separate interface that the AuthConfiguration extends, making it 1 definition.
If we don't extend it'll mean we'd need to have two instances of the config object.
sendClientId made optional
|
Any idea for a name ( 😄 ) for the Changed name to |
kadikraman
left a comment
kadikraman
left a comment
There was a problem hiding this comment.
Some minor suggestions, otherwise 👍
index.d.ts
Outdated
|
|
||
| export function refresh(config: AuthConfiguration, refreshConfig: RefreshConfiguration): Promise<AuthorizeResult>; | ||
|
|
||
| export function revoke(config: BaseAuthConfiguration, refreshConfig: RevokeConfiguration): Promise<void>; No newline at end of file |
There was a problem hiding this comment.
Surely
revokeConfig: RevokeConfiguration
index.d.ts
Outdated
| export interface AuthConfiguration extends BaseAuthConfiguration { | ||
| scopes: string[]; | ||
| redirectUrl: string; | ||
| aditionalParameters?: {[name: string]: any}; |
There was a problem hiding this comment.
Could you do
aditionalParameters?: {[name: string]: string};
as the values can technically only be strings?
index.d.ts
Outdated
| export interface AuthorizeResult { | ||
| accessToken: string; | ||
| accessTokenExpirationDate: string; | ||
| aditionalParameters?: {[name: string]: any}; |
|
Done, cheers! |
| @@ -0,0 +1,39 @@ | |||
| export interface AuthConfiguration extends BaseAuthConfiguration { | |||
There was a problem hiding this comment.
Minor nit here, the indentation seems to be off
index.d.ts
Outdated
| aditionalParameters?: {[name: string]: string}; | ||
| } | ||
|
|
||
| export interface BaseAuthConfiguration{ |
There was a problem hiding this comment.
can you move this to the top as AuthConfiguration extends it before it's being defined. Just for readability 👍
index.d.ts
Outdated
| export interface AuthConfiguration extends BaseAuthConfiguration { | ||
| scopes: string[]; | ||
| redirectUrl: string; | ||
| aditionalParameters?: {[name: string]: string}; |
index.d.ts
Outdated
| clientId: string; | ||
| } | ||
|
|
||
| export interface RevokeConfiguration{ |
There was a problem hiding this comment.
can we make sure that there's a space before {?
@kadikraman: Can you point prettier—I think it's set up with lint-staged, right?—at all TS files as well?
index.d.ts
Outdated
| export interface AuthorizeResult { | ||
| accessToken: string; | ||
| accessTokenExpirationDate: string; | ||
| aditionalParameters?: {[name: string]: string}; |
|
Had disabled prettier locally because it kept massively reformatting the package.json on edit. Now uses prettier formatting and added *.ts to lint-staged too. |
|
Looks good to me! Gj |
|
Looking good, landed this to master, will push it out in the next release. Thanks @janpieterz @kitten @kadikraman! |
5 participants
Fixed #47
First time adding definitions to a package, worked locally. Let me know if you want anything changed.