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
shibulijack 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 3 commits
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
5 changes: 3 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@
"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 .",
"build": "babel src/ -d lib/",
"buildw": "babel -w src/ -d lib/",
"build": "babel src/ -d lib/ --copy-files",
"buildw": "babel -w src/ -d lib/ --copy-files",
"test": "npm run build && node ./node_modules/mocha/bin/mocha --recursive",
"test-live": "npm run build && node ./node_modules/mocha/bin/mocha --require test_live/_supports/ci-safe --recursive test_live",
"prepublish": "npm run build"
Expand Down
67 changes: 67 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,67 @@
interface FBOptions {

@dantman dantman Jul 30, 2019

Copy link
Copy Markdown

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.

@shibulijack shibulijack Jul 30, 2019

Copy link
Copy Markdown
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<any>;
}

interface FBAPIInterface {
path: string;
method?: string;
params?: object;
callback?: Function;
}

interface FBKeyOrOptions {
key: string;
options: FBOptions;
}

interface SignedRequestResponse {
oauthToken: string;
userId: string;
userCountry: string;
}

interface UsageInterface {
callCount: number;
totalTime: number;
totalCPUTime: number;
}

export class Facebook {
Comment thread
shibulijack marked this conversation as resolved.
constructor(options?: FBOptions);

api(arguments: FBAPIInterface): Promise<any>;

@dantman dantman Jul 30, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.


extend(options: FBOptions): Facebook;

getAccessToken(): string;

getAppUsage(): UsageInterface;

getPageUsage(): UsageInterface;

getLoginUrl(options: FBOptions): string;

napi(arguments: FBAPIInterface): void;

options(keyOrOptions: keyof FBKeyOrOptions): any;

@dantman dantman Jul 30, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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


parseSignedRequest(signedRequest: string, appSecret: string): SignedRequestResponse;

setAccessToken(accessToken: string): void;

withAccessToken(accessToken: string): Facebook;
}

export const version: string;

export function FacebookApiException(res: Response): Error;

@dantman dantman Jul 30, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@dantman dantman Jul 30, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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


export const FB: Facebook;

@dantman dantman Jul 30, 2019

Copy link
Copy Markdown

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.