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

Support standard node callback pattern #7

Open
dhritzkiv opened this issue Aug 4, 2017 · 1 comment
Open

Support standard node callback pattern #7

dhritzkiv opened this issue Aug 4, 2017 · 1 comment

Comments

@dhritzkiv
Copy link

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.

@bolchowka
Copy link
Member

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.

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

No branches or pull requests

2 participants