Facebook-chat-api: Return a promise for login

[MikeShi42]

Proposal: The login function can return a promise so that it's easier to slip into a promise chain instead of promisifying it on the app side. The login function currently doesn't return anything.

Alternatively this could be resolved by the user, by just wrapping it in a promise. Or using node 8's util.promisify. Either of those solutions are fine and work, just that it adds additional code for everyone that wants to use promises.

[gamelaster]

That will probably break existing solutions... :-/

[MikeShi42]

I haven't poked too deep into the code, what would be broken with such a change? Or what external dependencies exist that would be broken? It seems like no one should be relying on the login function to return anything currently.

[Schmavery]

I don't have too much of a strong opinion on this change. If we can do it without breaking existing apis, I see no strong reason why we shouldn't include this.
I'm not personally a big promise-user, so I'm not sure what the correct approach is for this these days. Ideally we don't pull in a dependency on some bleeding-edge version of node or a large external library.
I'm also a little worried about the precedent it would set for other functions. We have a lot of functions that I could imagine people would want "promisified" if we did it for login. Probably not many of them return things, so that could be fine?

Can you give an example of usage of the api before and after your proposed change so I can get a better picture for the inconvenience this change would save people?

[demurgos]

Promises offer an simpler model for async tasks with a single result returned only once. It allows to decouple producer logic from consumer logic ("there is a return value"). Combined with the async/await syntax it allows much more readable code. Promises are supported since Node 0.12.

Compare both snippets:

function run() {
  // Create simple echo bot
  login({email: "FB_EMAIL", password: "FB_PASSWORD"}, (err, api) => {
    if(err) return console.error(err);
    console.log("Connected");
  });
}
run();

async function run() {
  // Create simple echo bot
  try {
    const api = await login({email: "FB_EMAIL", password: "FB_PASSWORD"});
    console.log("Connected");
  } catch (err) {
    return console.error(err);
  }
}
run();

You can express business logic at a high level and keep control about points were the function releases the control. You also get cleaner stack traces, etc.

[Schmavery]

@demurgos thanks for the example. Actually I'm familiar with the concept of promises, @MikeShi42 mentioned that there are promisify-style wrapper that can be used for people who currently want promises.
I was asking for an example of api code using the wrapper vs what it would look like built into the code. To get an idea of much of a hassle it currently is.

[demurgos]

I am mostly familiar with the Mongo, Postgres and the pg-promise libs, all exposing a dual callback / promise API.
Mongo and pg-promise and pretty heavy and let you deal with custom promise libraries: they introduced support for promises before it was widespread. Postgres introduced support for native promises but it does not use any wrapper, just deals with it manually. If you want to formalize this pattern, you can use the following function:

// If a callback is provided, call it with the result of `p` and return `undefined`.
// Else, just return the promise
function promiseOrCallback(p, cb) {
  if (!cb) {
    return p;
  }
  p.then((res) => cb(null, res), (err) => cb(err));
}

function login(opt, cb) {
  return promiseOrCallback(new Promise((resolve, reject) => {
    // Old code...
    someAsyncCall(..., (err, res) => {
      // More of the original code...
      // Only difference: replace cb(err) and cb(null, err) by these calls:
      if (err) {
        reject(err);
      } else {
        resolve(res);
      }
    ));
  }), cb);
}

[MikeShi42]

@Schmavery

On node 8 w/ util.promisify:

const login = require('facebook-chat-api');
const {promisify} = require('util');

const loginPromise = promisify(login);

Manual Wrapping w/ Native es6 Promises

function loginPromise(credentials) {
    return new Promise((resolve, reject) => {
        login(CREDENTIALS, (err, api) => {
            if (err)
                return reject(err);
            return resolve(api);
        });
    });
}

Realistically the effort isn't huge, and then using either version above vs. having it wrapped as a promise in the code base will be exactly the same. It's a nice to have, especially since it won't break existing implementations.

[Schmavery]

Sorry for the delay here and thanks for helping out answering my questions.
I think the takeaway is that if someone would like to make a (non-breaking) change to return a promise, then we'd be happy to merge it provided it doesn't introduce new dependencies on heavy packages or very new versions of nodejs. 😄

[hong4rc]

You can use node-facebook