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

Feature request: use node-style callbacks #124

Closed
psiphi75 opened this issue Apr 24, 2016 · 3 comments
Closed

Feature request: use node-style callbacks #124

psiphi75 opened this issue Apr 24, 2016 · 3 comments

Comments

@psiphi75
Copy link
Contributor

Currently bonescript uses value-only callbacks. I would like to make a feature request that uses node-style callbacks. This is the standard way that node uses callbacks.

I am glad to implement these changes. But I would like your approval / comments before continuing.

Below is some existing sample code from hw_mainline.js, it shows that the fs.readFile uses node-style callback, while bonescript converts this to a value-only callback.

    var readFile = function(err, data) {
        if(err) {
            resp.err = 'analogRead error: ' + err;
            winston.error(resp.err);
        }
        resp.value = parseInt(data, 10) / maxValue;
        callback(resp);
    };
    fs.readFile(ainFile, readFile);
    return(resp);

Below I suggest a way to migrate to node-style callbacks. Since this is likely a breaking change for many applications I have built in a warning message. This method has the following features:

  • node-style callbacks.
  • warns the user that their code may be broken.
  • the user can turn off the message by setting showNodeStyleCallbacksMessage to false. Note this message is just a sample, it could provide a link to a wiki with further documentation.
    var readFile = function(err, data) {
        if(err) {
            printNodeStyleWarningMessage('analogRead', callback);
            callback(err);
        } else {
            var value = parseInt(data, 10) / maxValue;
            callback(null, value);
        }
    };

// ... somewhere else...
var showNodeStyleCallbackMsg = true;
function printNodeStyleWarningMessage(fnName, callback) {
    if (showNodeStyleCallbackMsg && callback.length === 1) {
        winston.log(fnName + ' called with only one parameter, as of bonescript 0.5.0 you should use two, e.g. callback(err, value)');
    }
}

The difference is usage.

/* Before */
b.readAnalog('P9_12', function (resp) {
    if (resp.err) {
          console.error(resp.err);
    } else {
          console.log(resp.value);
    }
}

/* After */
b.readAnalog('P9_12', function (err, value) {
    if (err) {
          console.error(err);
    } else {
          console.log(value);
    }
}
@jadonk
Copy link
Owner

jadonk commented Apr 29, 2016

Why break the API?

@mvduin
Copy link

mvduin commented Apr 24, 2018

No need to break the API, you can just use callback.length to continue to support the old callback style for backwards compatibility in addition to the node.js standard callback style (which is required e.g. to be able to use util.promisify).

Note that only a few functions need this anyway. Most functions in bonescript (e.g. everything to do with gpio, pwm, and pinmux) are purely cpu-bound operations and using callbacks for them instead of using the synchronous versions accomplishes absolutely nothing besides making the code less efficient and harder to understand. Using callbacks for these should be entirely deprecated.

@jadonk
Copy link
Owner

jadonk commented Apr 25, 2018

This issue was moved to beagleboard#38

@jadonk jadonk closed this as completed Apr 25, 2018
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

3 participants