Typings/Typescript

[demurgos]

Hi, I created this issue to continue the discussion about typings/TS started at #219 without clobbering the other issue about the design questions.

I wrote a definition file for Typescript to import facebook-chat-api and benefit from compilation time type checks and provide auto-completion for editors. The definition is currently hosted at https://github.com/insa-frif/typed-facebook-chat-api and I propose to give you the ownership of this repository.

The definitions can be consumed with the typings defintion manager with the following entry in typings.json:

{
  "dependencies": {
    "facebook-chat-api": "github:insa-frif/typed-facebook-chat-api#b566427080717909d1ffae53b4910a0ebcb7e922"
  },
  "ambientDevDependencies": {
    "node": "registry:dt/node#4.0.0+20160412142033"
  }
}

Currently, you have to use the github:owner/repo syntax, but a PR is pending to add it to the registry and allow to use the command typings install --save facebook-chat-api.

Furthermore, I also proposed to use TS to write sources and automatically publish both ES5-compatible JS files and definitions. As a first step, I converted this repo naively on this v2 branch. As TS is just a superset of JS, it currently does not enforce strict typings, I ensured that everything compiles fine with a simple tsc command, without any warning. It mostly consisted at importing definitions for dependencies and using import statements. The biggest change is the buildApi method. The file structure is the same, it was just moved a step down to the src directory. The exposed interface for consumers is exactly the same as the one currently available. I need to do more tests to really ensure that the available API is exactly the same as the one currently available but it's a starting point. The reason why I propose to use TS - besides of emitting the definitions as discussed above - is to enforce compile-time checks (so refactoring is easier -> ie. spot where ErrorObjects should be replaced by real Errors), but also use ES6/ES7 features. Currently, the library uses lambda expressions and computed proporties for example, but it is not widespread in this lib (it felt to me as if it started to be used at a later point in time). A clean rewrite could benefit the whole library legibility and TS would be a nice reason to do a full review before the v2. There is also the obvious reason that it would allow the library to be future-proof by being able to already use the new JS syntaxes while maintaining compatibility with older versions so the following won't be an error anymore:

`6. I'm getting some crazy weird syntax error like SyntaxError: Unexpected token [!!!

Please try to update your version of node.js before submitting an issue of this nature. We like to use new language features.

What is your opinion about it ?

[Schmavery]

Thanks for submitting this issue for discussion. I think I need to think about it some more before giving an answer one way or another. I'm going to be on a trip for the next two weeks so it may be a little while before this gets fully figured out. Other people can feel free to discuss in my absence.

I'd be curious to know if anyone would switch to typescript or would be more likely to use this project if it supported typescript, so if you're one of these people, please chime in with your opinion!

A clean rewrite probably benefits any software project at any time, it's more a matter of investing the time and making sure you don't break anything :stuck_out_tongue:

I recognize the benefits of static typechecking, but if we really wanted that, there are multiple ways of getting it that would need to be considered. Flow is another option that comes to mind. Honestly typescript has seemed like a bit of a kludge to me but this probably isn't the best spot for a language debate :wink:

I definitely appreciate the work you've done here and we'll figure out some way of coming to a solution.

[demurgos]

Ok, thank you for your answer. Also, I would like to note that the definitions just have been added to the registry :tada:

They are now available via typings install --save facebook-chat-api

I will continue to improve them.

[demurgos]

Hi, I spent the last week writing/updating definitions for the dependencies for this project so the whole dependency tree should now be covered.

While I wrote these definitions, I had to check/read various documentations of varying quality. facebook-chat-api relies mostly on "big" modules so the quality was rather good, but there were still some non-fully specified behaviours. Some examples are the TrackerStreams used by npmlog not exposing the isFinished() method while the documentation states otherwise, or the very vague documentation about request's cookie handling. (According to it, it relies on tough-cookie but they add a wrapper and the available options are not specified). This module is well documented but still has some "non blatantly clear" result descriptions. The documentation often refers to IDs, threadIDs, userIDs and so on without specifying upfront what they really are. I got an answer on my typings commit but it starts with "we generally try to" so it's not guaranteed to be fully normalized across the whole app. (They can be numbers or strings when used as an argument, but when in a result they should be strings) I do not try to dismiss all what you made (it's even the opposite, I would like to you make this module rock-solid), but it's hard to be fully unambiguous when documenting code so the ability to directly annotate the code (as a "single source of truth") and then expose these metadata is a really good thing. Still about the guarantees, I plan to use a database soon in a project using facebook-chat-api, even if checks are performed by the database, I have to know which types are exposed by your API to build my schema/eventually convert the data. Some examples are the documented "timestamp"s: is it a number ? is it a string representation of the number ? is it a Date object ? What about the data attached to error objects ? My point is that even without compilation-time static checks, type annotations ARE useful.

Now that I finished typing all of the dependencies, I am starting the second phase of the conversion by adding type annotations on functions. (The first phase was just updating JS files to TS and getting sure that it compiles/builds fine and stays compatible with the current API - no breaking change for the moment :smiley:). I am keeping my v2 branch in sync with this repo by merging the fixes landing here.

[bsansouci]

Hey @Demurgos, this is amazing. Thanks a lot for this work because, as you said, we say "we generally try to" but have no guarantees ourselves. I think @Schmavery expressed our opinion about TS. We don't have use for it directly, but the API and the users would benefit from better types. Apart from the fact that neither @Schmavery or I are experts at TS, we also like to avoid dependencies and especially dependencies on preprocessors because of the potential conflicts that can arise. From what I know, this module should work on the client-side (as a chrome extension really...) as long as you can mock request among other things. Then we could have only a .d.ts file which would describe the types of our API, but we'd have to keep it in sync. My opinion is that if enough people want TS support, then we should have the definition file. We don't really have plans to make huge changes anymore, so the burden of keeping the two in sync isn't that huge. That said I think @Schmavery would argue against but I'll let him explain his opinion when he comes back.

BUT it's possible that we can find a good middle ground. Now that you've figured out all of the input/output types we should be able to update our docs and add inline comments to explain the API. Also I want to fix those uncertainties that you have to make them guarantees. I think I'm ok with breaking things as long as we can have better guarantees.

• IDs are always strings when we return them, we try really hard to guarantee that (and I think we've done a pretty good job at it so far)
• Dates are always int unix epoch

What is the problem with the data attached to errors (we already have a tracking issue about errors)?

Also yes, I had to deal with CookieJar being a little derpy in the past but I'm pretty sure we're using it right so far.

[demurgos]

Thank you for your answer. I'll dig more into the exposed interfaces during the next days so I'll let you know if I find something.

Apart from the fact that neither @Schmavery or I are experts at TS, we also like to avoid dependencies and especially dependencies on preprocessors because of the potential conflicts that can arise. From what I know, this module should work on the client-side (as a chrome extension really...) as long as you can mock request among other things.

I'd just like to clarify one thing: a module written in Typescript does not require Typescript to be used: any ES5 (or even ES3 ?) environment can run its emitted js files. I did not checked it out with git because it's build automatically, but the module is emitted and published to npm as normal ES5 Javascript. These Javascript file do not require any particular environment and work fine in native browser environments. (see my point in the first comment about actually being able to use the latest ES6/7 features without breaking compatibility with older clients). It really just splits the *.ts files by sending the implementations to a *.js file and the type informations to a *.d.ts file, and you can use the *.js without the *.d.ts. Could you expand what you mean about "potential conflicts" ?

I know that I may push it a bit too hard and I don't want to completely mess with your project, but I definitely will keep you informed of my progress :). I was rather reluctant at first about introducing a build step in my own projects (even if it is supported by most IDEs and you can watch files to build automatically), I had some rough cases where I had to do some re-factoring and there are some restrictions when pushing it to its limits; but I just felt that it lead to an overall improvement of my code so that's why I'm trying to share my enthusiasm.

What is the problem with the data attached to errors (we already have a tracking issue about errors)?

I am thinking about the following throw statements throwing an arbitrary object with an error property: what are its other properties ? (of course I could track down the formatter function myself, but I think that we agree that the properties of error objects should be documented).

• https://github.com/Schmavery/facebook-chat-api/blob/d63b7776d615540096ddd128998336287f6d9618/src/searchForThread.js#L24
• https://github.com/Schmavery/facebook-chat-api/blob/d63b7776d615540096ddd128998336287f6d9618/src/changeThreadColor.js#L21
• https://github.com/Schmavery/facebook-chat-api/blob/d63b7776d615540096ddd128998336287f6d9618/src/getUserID.js#L39

There are certainly other parts of the code, but since your explanation to not use native Errors was to provide additional data, this data should be documented somewhere.

[krokofant]

Nice! It would be sweet to have something like this maintained in this repo to provide IntelliSense out of the box via npm. I started to write my own definition when I found this.

To find what the structure of the api.getThreadHistory method is turned out to be cumbersome but with the definition file it became very clear. Without these types of definition files, what is your workflow?

Suppose you'd want to do something like:

login(credentials, function (err, api) {
  if (err) return console.error(err)
  api.getThreadInfo(threadID, (err, info) => {
    if (err) return console.error(err)
    api.getThreadHistory(threadID, 0, info.messageCount, Date.now(), (err, data) => {
      if (err) return console.error(err)
      console.log(data[0].timestampRelative)
    })
  })
})

Do you just have the history data structure in your head and if so based on what?

[demurgos]

Do you just have the history data structure in your head and if so based on what?

Based on retro-engeneering ?

I went further and rewrote the whole module to TypeScript but couldn't keep up with the updates. The typings should still be good enough, but for the module I'd need more free time.

Here is my branch (6 months behind) if you're interested in helping on this. (See src/interfaces/fb-*.ts for the Facebook data)

[bsansouci]

@krokofant Looking at formatMessage for example, we tried to make sure the shape of the object was fully defined and clear. The docs also mentions all of the accessible fields and sometimes what they mean. The biggest reason why there might not be an explanation of some field in the response is biggest we have no idea why it's there but we thought maybe someone could make sense of it.

[krokofant]

@bsansouci Thank you the the insight!

[AllanWang]

Will this still be updated? There have been a few additions since 9 months ago that might be worth adding to the typings

[Schmavery]

@AllanWang neither @bsansouci or I use typescript, so it hasn't been as much of a priority to keep these up to date as it has been to fix bugs in the API. I'm not personally interested in converting the API to typescript, as I don't think we gain much from it (within the context of the API, not consumers of it) and without doing that, maintaining the definitions file seems like non-trivial overhead. Doesn't help that there are multiple (incompatible) type systems for JavaScript. If we support typescript, should we also support users who want to use flow? Last I checked, there wasn't a converter between them. Just my $.02. Not sure if I'll change my mind, but always interested to hear people's opinions.

[demurgos]

Hi, Thank you for your notifcation. I was pretty busy lately, but I'll try to find some time to update the type definitions. (As a side note: it may be worth adding a "CHANGELOG.md" file for this exact situation)

About my opinion on Typescript, I already explained in my first why it may be worth considering Typescript. One of the big changes since this post is the release of the version 2.1 with ES3/ES5 support of async functions: it is a joy to use. About the multiple type systems, I would say that supporting one is still better than none and Typescript seems to have more traction and features than Flow.

[daomtthuan]

@demurgos Hello, my first apology is because my English is not good. Thanks for your work last time, but I have a bit of opinion that the Typings package on NPM is outdated and switches to @types. Would you publish this definition on @types?

[demurgos]

I am no longer using this library or updating typings. You can fork the types and send a PR to this repo or @types.