Added type definitions to make it Typescript compatible by shibulijack · Pull Request #176 · node-facebook/facebook-node-sdk

shibulijack · 2019-07-30T06:28:11Z

No description provided.

dantman

If you're going to add TypeScript definitions, could you add some testing code to validate that they are correct. IIRC we have CI and it would be preferable to have the CI make sure no-one breaks the typedefs.

dantman · 2019-07-30T06:44:12Z

src/fb.d.ts

+
+    setAccessToken(accessToken: string): void;
+
+    withAccessToken(accessToken: string): any;


This should also be Facebook

dantman · 2019-07-30T06:46:25Z

src/fb.d.ts

+
+    napi(...args): Promise<any>;
+
+    options(keyOrOptions: FBOptions): FBOptions;


This needs overloading for the other ways it can be called.

keyof may be useful here.

dantman · 2019-07-30T06:47:19Z

src/fb.d.ts

+
+    getLoginUrl(): string;
+
+    napi(...args): Promise<any>;


napi does not return a promise, it accepts a node style callback.

dantman · 2019-07-30T06:50:31Z

src/fb.d.ts

+export class Facebook {
+    constructor(options: FBOptions);
+
+    api(...args): Promise<any>;


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

dantman · 2019-07-30T07:00:29Z

src/fb.d.ts

+
+export const version: string;
+
+export function FacebookApiException(res: Response): void;


Where is Response defined? void also is wrong, this is a subclass of Error.

Response is from TypeScript lib.dom.d.ts.

dantman · 2019-07-30T07:00:45Z

src/fb.d.ts

+
+export function FacebookApiException(res: Response): void;
+
+export const FB: Facebook;


This is also a default export.

src/fb.d.ts

dantman · 2019-07-30T07:05:54Z

package.json

  "contributors": [
-    "Daniel Friesen <d@danf.ca> (http://danf.ca)"
+    "Daniel Friesen <d@danf.ca> (http://danf.ca)",
+    "Shibu Lijack <shibulijack@gmail.com> (https://github.com/shibulijack"


Missing ), if you're going to make yourself the only one of the many people who has submitted PRs to this library attributed here.

Sorry but I don't understand. I'll fix the missing parenthesis but am I missing anything else here?

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.

Alright. I'll remove myself from the contributor list.
Just curious though, shouldn't all the 16 people be included in the contributor list?

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.

dantman · 2019-07-30T10:18:58Z

src/fb.d.ts

+
+    napi(arguments: FBAPIInterface): void;
+
+    options(keyOrOptions: keyof FBKeyOrOptions): any;


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

dantman · 2019-07-30T10:23:25Z

src/fb.d.ts

+export class Facebook {
+    constructor(options?: FBOptions);
+
+    api(arguments: FBAPIInterface): Promise<any>;


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.

dantman · 2019-07-30T10:25:24Z

src/fb.d.ts

+
+export const version: string;
+
+export function FacebookApiException(res: Response): Error;


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.

Also I'm pretty sure Error extensions are implemented using class and extends instead of functions.

Added type definitions

e3d3b89

dantman requested changes Jul 30, 2019

View reviewed changes

shibulijack added 2 commits July 30, 2019 12:46

Updated build commands

1aa43a5

Addressed review comments

e4bd527

dantman reviewed Jul 30, 2019

View reviewed changes

More review comments

3d1933b

shibulijack force-pushed the master branch from 1c999de to 3d1933b Compare July 30, 2019 12:24


		setAccessToken(accessToken: string): void;

		withAccessToken(accessToken: string): any;


		napi(...args): Promise<any>;

		options(keyOrOptions: FBOptions): FBOptions;


		export const version: string;

		export function FacebookApiException(res: Response): void;


		export function FacebookApiException(res: Response): void;

		export const FB: Facebook;


		napi(arguments: FBAPIInterface): void;

		options(keyOrOptions: keyof FBKeyOrOptions): any;

Conversation

shibulijack commented Jul 30, 2019

Uh oh!

dantman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants