Added type definitions to make it Typescript compatible #176
Conversation
src/fb.d.ts
Outdated
|
|
||
| setAccessToken(accessToken: string): void; | ||
|
|
||
| withAccessToken(accessToken: string): any; |
src/fb.d.ts
Outdated
|
|
||
| napi(...args): Promise<any>; | ||
|
|
||
| options(keyOrOptions: FBOptions): FBOptions; |
There was a problem hiding this comment.
This needs overloading for the other ways it can be called.
keyof may be useful here.
src/fb.d.ts
Outdated
|
|
||
| getLoginUrl(): string; | ||
|
|
||
| napi(...args): Promise<any>; |
There was a problem hiding this comment.
napi does not return a promise, it accepts a node style callback.
src/fb.d.ts
Outdated
| export class Facebook { | ||
| constructor(options: FBOptions); | ||
|
|
||
| api(...args): Promise<any>; |
There was a problem hiding this comment.
This does not look like valid TypeScript.
Also even though DefinitelyTyped discourages it, we might want to consider using a generic here instead of any.
src/fb.d.ts
Outdated
|
|
||
| export const version: string; | ||
|
|
||
| export function FacebookApiException(res: Response): void; |
There was a problem hiding this comment.
Where is Response defined? void also is wrong, this is a subclass of Error.
There was a problem hiding this comment.
Response is from TypeScript lib.dom.d.ts.
|
|
||
| export function FacebookApiException(res: Response): void; | ||
|
|
||
| export const FB: Facebook; |
package.json
Outdated
| @@ -9,7 +9,8 @@ | |||
| ], | |||
| "author": "Thuzi LLC <[email protected]> (https://github.com/Thuzi)", | |||
| "contributors": [ | |||
| "Daniel Friesen <[email protected]> (http://danf.ca)" | |||
| "Daniel Friesen <[email protected]> (http://danf.ca)", | |||
| "Shibu Lijack <[email protected]> (https://github.com/shibulijack" | |||
There was a problem hiding this comment.
Missing ), if you're going to make yourself the only one of the many people who has submitted PRs to this library attributed here.
There was a problem hiding this comment.
Sorry but I don't understand. I'll fix the missing parenthesis but am I missing anything else here?
There was a problem hiding this comment.
No, I was just noting that there are 16 other people who have contributed code to this library and none of them are credited in the contributors list. It's really only listing me because I took over as maintainer.
There was a problem hiding this comment.
Alright. I'll remove myself from the contributor list.
Just curious though, shouldn't all the 16 people be included in the contributor list?
There was a problem hiding this comment.
Perhaps, but in general I think most npm packages bother adding everyone to the contributors list. I sometimes see a CONTRIBUTORS file. But the contributors list is usually kept short, presumably because it's downloaded by everyone that installs the library.
src/fb.d.ts
Outdated
|
|
||
| napi(arguments: FBAPIInterface): void; | ||
|
|
||
| options(keyOrOptions: keyof FBKeyOrOptions): any; |
There was a problem hiding this comment.
No, this needs overloading, not another interface type.
Something like:
options(): FBOptions
options(key: K): ... // (I'm not sure which to use here but there should be a way to use keyof and also get TS to automatically infer the correct return type
options(options: FBOptions): void
src/fb.d.ts
Outdated
| export class Facebook { | ||
| constructor(options?: FBOptions); | ||
|
|
||
| api(arguments: FBAPIInterface): Promise<any>; |
There was a problem hiding this comment.
api and napi args are not a single arguments object, they're separate ordered arguments.
You'll need overloading here as well to differentiate between path, cb and path, params, cb, etc and also to differentiate between the callback form and the form without the callback that returns a promise.
src/fb.d.ts
Outdated
|
|
||
| export const version: string; | ||
|
|
||
| export function FacebookApiException(res: Response): Error; |
There was a problem hiding this comment.
res is not a fetch Response instance. It's an object returned by the Facebook API in the format that Facebook specifies for error responses.
There was a problem hiding this comment.
Also I'm pretty sure Error extensions are implemented using class and extends instead of functions.
No description provided.