Version 2.0 Proposal and Feedback

[Schmavery]

Hello everyone, looking to get some public opinion. We've been struggling a while to maintain the same signature on message objects (the objects received by listen). Classically, these have contained a lot of other useful information about the thread such as the participants, their IDs, the name of the thread etc. However, as some of you know, this has forced us to have a special code path for threads with a number of participants >5, because for threads this big, facebook doesn't provide the full list of participants. This means we actually do an extra network request every time you receive a message from a thread this large. Caching this result has been considered in the past but not implemented because of the increased complexity cost, not to mention unpredictable memory/network costs etc.

My proposal is that we make a breaking change to the API to reduce message objects to a much more compact form. This signature is chosen for maximum backwards compatibility, but removes some redundant timestamps, location, and more notably, the participants information and thread name. The attachments would likely be cleaned up a little as well, given that they also contain a fair bit of arguably useless information, but I haven't looked into this as deeply yet.

{
    type: "message",
    senderID: "00000000000",
    body: "Hello World",
    threadID: "00000000000000",
    messageID: "0000000000000000000000",
    attachments: [],
    timestamp: 000000000,
}

The missing information would be retrieved on demand from the already proposed getThreadInfo function (#209). This way, users of the API can decide whether or not they need this information and use whichever caching strategy they want, if desired.

What do people think? How hard would this break your current use of the API? I would consider adding deprecatedListen to setOptions or something similar that would make an extra network call for every message if it was really desired.

Other changes:

• Custom Errors #146

[Unpaid8503]

I think it's a good idea

[gamelaster]

Well, that is good idea + caching is needed I think, its can reduce a requests with same point.

[andrecordeiro]

Very good!

[Schmavery]

@GAMELASTER the entire point here is to avoid caching.

[hagemt]

:+1: That seems like a completely rational choice.

[AllanWang]

Will there still be a way of accessing sender names? Or will it be done by getting the id and then the full name?

[Schmavery]

If you have sender IDs, you can always get sender names with one extra call to getUserInfo, can't you?

[AllanWang]

Yes I can. This is truly lazy, but would you guys consider keeping senderName but having it do those two calls instead? Or should we just make a function now that does that for us. (Not too experienced with coding yet so apologies if this is an incorrect question)

[mehcode]

There is no sense to double up network load if the client doesn't need the extra info. I'd be okay with a helper method on the object.

[Schmavery]

I just realized that I should add a boolean field isGroup to the new message signature, since that's probably what most people were using participantIDs for anyway.

[demurgos]

Hi ! It's great to see that this module is really active. Since you are planning to introduce some breaking changes, I'd like to know if it would be possible for this module to follow the strict commonjs specification for its exports to be fully compliant with ES6. Concretely, since last summer with the official release of ES6 or the rise of Typescript with major libraries like Angular, it might be a good thing to use a standard way to import this module. Currently, the problem is that Node allows the export object to be a function when both commonjs and ES6 only allow a plain namespace object.

Concretly, it would change the export line:

// currently:
// module.exports = login;
// ES6-compatible:
exports.login = login;

It would allow to consume it as:

import {login} from "facebook-chat-api";

And ES5 users could still do:

var fbChatApi = require("facebook-chat-api");
fbChatApi.login(...);

The reason why I am talking about it, is that I really like this module so I made a Typescript definition file for this module so it can provide type-checks during compilation and help IDE's autocompletion. Because of the current interface of the module, there are unfortunately limitations because the sole export can be a function (it's quite hacky to work with ES5 modules) whereas with an ES6 compliant interface, it could be possible to also provide some type or object interfaces for external consumption.

// For example: (but it can be much more)
import {login, Message, FacebookChatApiError} from "facebook-chat-api";
// login -> still the same: the function to get the api
// Message -> an interface for message
// FacebookChatApiError -> the constructor of the errors thrown by the module
// with its prototype as the native Error to allow:
// err instanceof Error
// err instanceof FacebookChatApiError

In any case, would you be interested in hosting the definitions on this repo ?

While I am still at it, if you're interested I could also help you to easily convert this module to Typescript (with all the benefits that come to it such as type-safety, ES6 features and automatically generated external typings). I'll try it on a separate branch.

Finally, there is one last thing that bugged me with this lib: why are you throwing some custom object ({error: string}) instead of real errors ? Would it be possible to use real errors ? (It's quite surprising to see this)

[demurgos]

Here are the definitions. It's not finished, but it's already pretty complete.

[Schmavery]

Yeah, error handling is currently known to be a mess.. We've been partly using #146 to track this, but we haven't gotten around it yet. The errors being thrown should probably be all made consistent, and I think there are also some bigger problems with promise/callback logic polluting stack traces.

As far as the rest... I'd probably have to think about it before making this change. I've gone down the typescript road in the past and I'm not convinced that it offers enough to make changes for the sake of itself. That said, if it doesn't take much to support it, I have nothing against it. I'm not really convinced that for this particular repo, using js, we would ever need to export multiple things. This is also a very different kind of breaking change. With the changes we just in messages, we removed a couple things out of necessity but for the most part, the api stays the same. This breaks import, which means that anyone who uses it will be totally broken until they fix it..

I do want to say that it's totally awesome that you like this project enough to try and contribute something like this. Hopefully we can find a way to make everyone happy :tada:

Another thing to note is that now that facebook has released their new official chat api, it's likely that support and usage of this version may taper off.

[demurgos]

Ok, thanks for your answer. I'll try to find a way to keep the same way of exporting, I'll notify your later today.

Could you post a link to the new official API ? I've heard about an API for bots, but what about user accounts ?

[Schmavery]

Unfortunately that's the one I was talking about, there isn't one for user accounts.

[demurgos]

So your module is still very valuable :tada:

I was able to export both the value (function) and the interfaces so it's no longer that crucial to implement an ES6 interface (but keep it in mind for v3.0 :smile: ) The new declaration allows:

import * as fbChat from "facebook-chat-api";

// you can use it as namespace for interfaces:
let credentials: fbChat.Credentials = {email: "foo", password: "bar");

// you can use it as a function:
fbChat(credentials, (err: fbChat.ErrorObject, api: fbChat.Api) => {
  api.getCurrentUserID();
  // and so on
});

(It's getting a bit off-topic so I'll open a new issue when it's fully ready)

I'll see now if it's possible to quick convert it to TS and maybe help you clean the error handling.

[bsansouci]

Hey @Demurgos, this is super awesome! Thanks for this hard work. I'd be ok with merging this in for whoever feels like using TS, but I don't know how it works / how to set it up for people who want it but not have it in people's face for whoever doesn't want it.

About errors, the main reason we made an object instead of throwing new Error() was that we could attach any kind of information to that object without causing trouble, but also because FB's error had the same form {error: ...} so we could just throw FB's error and not worry too much about it. As @Schmavery said, we totally messed up how we handle it and we really need to fix it (if you throw an error inside a callback and don't catch it, it'll bubble up to our catch and that'll make another call to your callback happen with the error.

[Schmavery]

What about hosting the definitions on something like DefinitelyTyped? Is that not a normal way of solving this problem? I'm not completely familiar on best practices for ts definition files.

[demurgos]

Hi, I almost finished my try to convert this module to TS, and by the way I had the occasion to thoroughly review the whole source code. Overall it's well put together but there are some hacky parts and rough corners: I would like to help (I've seen a lot of TODOs).

About the errors, I effectively saw that there are places in utils.js or src where you add more data to the error object. I had the exact same problem as you, so I wrote a simple module to throw errors while attaching data to them (and quickly override the error name e or wrap an other error). You may take a look at Incident It's a pretty straight forward drop-in for Error:

// utils.js
try {
  res = JSON.parse(makeParsable(data.body));
} catch(e) {
  throw {
    error: "JSON.parse error. Check the `detail` property on this error.",
    detail: e,
    res: data.body
  };
}

// the same with incident
try {
  res = JSON.parse(makeParsable(data.body));
} catch(e) {
  throw new Incident(e, {res: data.body}, "JSON.parse error.");
}
// and on the other side:
console.log(error.cause); // the original error
console.log(error.data.res); // data.body
console.log(error.message); // "JSON.parse error"

About DT, it had some issues because it registered every module at the global level so it lead to numerous name collisons and errors. Another problem was the centralized huge repository with every single definition. The team behind DT created a new project (typings) to solve name conflicts between dependencies and have a distributed network. It's fully compatible with DT and pretty stable. I made a PR to the registry, but my proposition was about giving you the ownership of these definitions. At its bare minimum, it just requires a repo to expose an index.d.ts and a typings.json file.