beagleboard / bonescript

Scripting tools for the BeagleBoard and BeagleBone
http://beagleboard.org
MIT License
32 stars 9 forks source link

Feature request: use node-style callbacks #38

Closed jadonk closed 4 years ago

jadonk commented 6 years ago

From @psiphi75 on April 24, 2016 0:4

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:

    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);
    }
}

Copied from original issue: jadonk/bonescript#124

jadonk commented 6 years ago

Why break the API?

jadonk commented 6 years ago

From @mvduin on April 24, 2018 5:3

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.