api-client-nodejs icon indicating copy to clipboard operation
api-client-nodejs copied to clipboard

Support standard node callback pattern

Open dhritzkiv opened this issue 7 years ago • 1 comments

Currently, it seems that the callback only ever has one argument, with that sole argument representing either a result, or an object with a potential error in it:

api.goals.list(params, (result) => {
    if (result.error) {
          throw result.error;
    }

    //else result is the data we're looking for 
});

Ideally, the pattern would use the standard node callback pattern of (error, result):

api.goals.list(params, (err, result) => {
    if (err) {
          throw err;
    }

    //else result is the data we're looking for 
});

An advantage to this (other than predictable behaviour for new developers integrating this module) is easy compatibility with node v.8.x.'s promisify method to make methods into Promises*:

const asyncGetGoalsList = promisify(api.goals.list.bind(api));

versus the current:

const asyncGetGoalsList = (params) => new Promise((resolve, reject) => {
    api.goals.list(params, result => {
        if (result.error) {
            if (result.body) {
                  result.error.body = result.body;
            }

            return reject(result.error);
        }

        return resolve(result);
    });
});

*That being said, it'd also be nice if this library supported Promises in addition to callbacks.

dhritzkiv avatar Aug 04 '17 18:08 dhritzkiv

Thank you for this suggestion. I agree that Promises should be supported by our library out of the box. Also, we can do better with standard error handling.

We will probably fix those issues when we release some updates to this lib. We don't have this planned in the nearest future, though.

bolchowka avatar Aug 07 '17 08:08 bolchowka