Skip to content
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.

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

Added basic Typescript definitions. #46

Closed
wants to merge 10 commits into from
Closed

Added basic Typescript definitions. #46

wants to merge 10 commits into from

Conversation

andrewstart
Copy link

Added a basic Typescript definition file. I am relatively new to Typescript, so I don't know how to improve it from here.

@dantman
Copy link

dantman commented May 21, 2016

It's a start, but the interface has changed quite significantly. A proper Typescript definition should include the Facebook class.

@andrewstart
Copy link
Author

Updated to have all of the module exports and have the .api() Promises.

@dantman
Copy link

dantman commented Jun 6, 2016

I'm still not sure how I should add tests for this to ensure it always works.

But in the meantime here are some notes:

  • The FB.{api,napi}(path, method, cb?) form is also valid (ie: +method but without params)
  • Missing extend and withAccessToken
  • Options to getLoginUrl are technically optional
  • Bonus: A HTTPMethods enum could be made for the method argument
  • Bonus: You should be able to define a FacebookOptions interface with the list of option names and their type, that can then be applied to the return type of options() and to options(optionsDict: any)

@dantman
Copy link

dantman commented Jun 6, 2016

typescript-definition-tester may work for adding tests for this.

@andrewstart
Copy link
Author

I've updated the definitions as per your suggestions, and also threw in an interface for getLoginUrl().

I also added a test that uses typescript-definition-tester. The test uses ES6 import syntax. I tried to set up an additional file to test that uses require() as well, but I haven't found a way for a TypeScript module to have both the ES6 and CommonJS exports. As such, TypeScript users should stick to the ES6 imports, because that's what this definition is set up for.

@@ -0,0 +1,61 @@
import Promise = require("any-promise");
Copy link

Choose a reason for hiding this comment

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

You can remove this line. Promise can be used directly without importing it as it is part of ES6.

Copy link
Author

Choose a reason for hiding this comment

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

While that is true, that doesn't mean that everyone is using an ES6 environment, and additionally this library uses any-promise as the Promise implementation.

Copy link

Choose a reason for hiding this comment

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

That's how i have seen it used in most well written typings I have used. If you are not targeting ES6, it is up to the consumer to provide the typings for Promise. Example from a fairy recent declaration https://github.com/facebook/dataloader/blob/master/src/index.d.ts

Copy link

@ibratoev ibratoev Feb 8, 2017

Choose a reason for hiding this comment

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

Additionally, 2.0 of this library supports only NodeJS environments that support Promise natively ;)

@ibratoev
Copy link

Btw, my comment is minor and not a blocker. I am 👍 for merging the typings. Tested it locally - all works fine ;) Thanks for that, @andrewstart ;)

@dantman
Copy link

dantman commented Feb 24, 2018

https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html seems to recommend publishing types through DefinitelyTyped/@types instead of as part of the package.

In any case I still want to get flow working first. Maybe we could event convert that to typescript definitions.

@yaroslav-ilin
Copy link

So, no plan to get this merged any time soon?

@dantman
Copy link

dantman commented Mar 25, 2018

@nilfalse The code in this PR isn't even up to date, the API has changed since this was written.

@dantman
Copy link

dantman commented Apr 2, 2018

This PR is out of date, i.e. it will not work anymore. And it does not attempt to handle Flow. #167 documents plans regarding both Flow and TypeScript. The code in this PR may be helpful in the future as a reference in for writing up to date typings, but for now I am closing it.

@dantman dantman closed this Apr 2, 2018
@hyperactivepuss
Copy link

hyperactivepuss commented Mar 21, 2019

pzdc, why this pr wasn't accept?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants