Skip to content

Added type definitions to make it Typescript compatible #176

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Added type definitions to make it Typescript compatible #176

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3d3b89
Added type definitions
Jul 30, 2019
1aa43a5
Updated build commands
Jul 30, 2019
e4bd527
Addressed review comments
Jul 30, 2019
3d1933b
More review comments
Jul 30, 2019
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
4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

],
"homepage": "https://github.com/node-facebook/facebook-node-sdk",
"license": "Apache-2.0",
Expand All @@ -18,6 +19,7 @@
"url": "https://github.com/node-facebook/facebook-node-sdk.git"
},
"main": "./lib/fb.js",
"types": "./lib/fb.d.ts",
"scripts": {
"precommit": "lint-staged",
"lint": "eslint .",
Expand Down
39 changes: 39 additions & 0 deletions src/fb.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
interface FBOptions {
Copy link

Choose a reason for hiding this comment

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

A bunch of options appear to be missing.

Copy link
Author

Choose a reason for hiding this comment

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

I used the ones mentioned in the README. Turns out the list wasn't exhaustive. I'll update the options with the ones listed in the source code.

accessToken: string;
appId: string;
appSecret: string;
version?: string;
proxy?: string;
timeout?: number;
scope?: string;
redirectUri?: string;
Promise?: Promise;
}

export class Facebook {
constructor(options: FBOptions);

api(...args): Promise<any>;
Copy link

Choose a reason for hiding this comment

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

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.


extend(options: FBOptions): Facebook;

getAccessToken(): string;

getLoginUrl(): string;

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

Choose a reason for hiding this comment

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

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


options(keyOrOptions: FBOptions): FBOptions;
Copy link

Choose a reason for hiding this comment

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

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

keyof may be useful here.


parseSignedRequest(signedRequest: string, ...args): Object;
Copy link

Choose a reason for hiding this comment

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

Doesn't look valid and should have something better than Object, which isn't even good TS practice.


setAccessToken(accessToken: string): void;

withAccessToken(accessToken: string): any;
Copy link

Choose a reason for hiding this comment

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

This should also be Facebook

}

export const version: string;

export function FacebookApiException(res: Response): void;
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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


export const FB: Facebook;
Copy link

Choose a reason for hiding this comment

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

This is also a default export.