Explore Promises in addition to callbacks

[reconbot]

Usurps #261

We'll be dropping older nodes around version 6, we currently polyfill promises for version 4, so we got them to use.

We'd need tests along with any PR

[reconbot]

A problem with promise based apis is that it is not compatible with the optional callback / error event style approach we've adopted;

// Current api
let port = new SerialPort('/bad-port', {autoOpen: false});
port.on('open', () => console.log('yay the port is open!'))
port.on('error', () => console.log('boo we had an error!'))
port.open();  // no callback
// boo we had an error!

port.open(function(err) { if (err) { console.log("I'm dealing with this error") }});
// I'm dealing with this error

In node 6x+ unhandled promises throw globally, in all nodes unhandled error events throw globally. We're still going to support 4x. This might not be something we need to worry about.

So if also always return a promise or only return a promise when not given a callback, and we don't handle it's rejection some nodes will throw. And if we don't provide a callback and don't handle the error event, then all nodes will throw.

All our functions throw when given bad arguments, there seems to be a consensus around rejecting promises instead.

I think this all means that we'd have to switch wholesale to promises instead of callbacks.

[reconbot]

There is an alternative approach which we can take, and that is to do what nodejs core is eventually going to do. Providing functionAsync equivalents that return promises.

nodejs/node#5020

That's probably the way to go.

[reconbot]

This isn't on the roadmap for now.

[adamreisnz]

Hi @reconbot , any chance of revisiting this, or accepting PR's for work on this?

Currently we are forced to use Bluebird to promisify the port object, and call port.openAsync etc. This is of course a bit redundant, especially now that Native promises have landed in Node and are pretty much mainstream.

When I rewrote the Node API for Sendgrid, we also introduced promise support while maintaining callbacks for backwards compatibility. I see the issue with not passing a callback and not handling rejected promises, which could trip up apps using callbacks. However, given the speed at which you are releasing major versions with other breaking changes, it's conceivable that a new major version is released that either always expects promises/async to be used.

Alternatively, have you considered a flag that enabled promises globally (or during instantiation of the SerialPort object)?

const port = new SerialPort(port, {
  baudRate,
  autoOpen: false,
  usePromises: true,
});

This could even be the default setting, with optional false value indicating the current style of callbacks only usage.

Thoughts?

[reconbot]

The next major version is unfortunate. It fixes some major windows performance issues and changes our binary distribution system. Doing the bump seemed worth it even though there's only potential behavior changes.

As for promises. I started using them for the bindings but one of the goals was to match the node stream api as close as possible. In fact most of the "magic" is in the bindings. I'm doing an exploration of other kinds of streams (whatwg/streams, most) and async iterators. But I doubt we'd switch this project to any of them, but instead share the bindings with other packages. (and probably split them into their own package)

So async methods for node serialport? The convention doesn't exist for node streams but we snuck it into #list().

• .set(), .get(), .flush(), .drain(), .update(), .open(), and .close() could return a promise if no callback is provided without issue.
• .read() and .write() already have useful return values and are stream methods. A .writeAsync() would be trivial but a .readAsync() would be difficult to do and would only work on paused streams. I've tried to write a utility function .readAsync() for use on generic streams a few times.
• .pause() and .resume() are sync and also stream methods, no changes there.

It seems that node core is moving towards the .fooAsync() style of promised based callbacks. I think it makes sense to stay in "spec". I want to have more discussion about it but I think it makes sense to accept PRs with async prefixed methods. (We should probably fix .list() and make a .listAsync() too.)

PS I'd love to make a non node-streams version of serialport. And I love async/await, a lot.

[adamreisnz]

Thanks for the detailed comments!

Since I use the .on('data') event instead of .read(), for me the main benefit would arise from making .open() and .close() return promises. If the methods you mentioned in your first bullet point could return promises when no callback is provided that'd be great.

I wouldn't mind if the API is split between callback and non-callback functions, but I think the naming would be misleading. E.g. .open() will still take a callback, and it will not be sync, but it might be implied by the presence of an .openAsync().

So I would prefer the approach where .open() returns a promise when no callback is provided. Can probably leave read, write, pause, resume as they are for now.

And I love async/await, a lot

Amen!

[reconbot]

I'm wrong on the fooAsync() in core. They seem to be landing on a module namespace. eg require('serialport/promise') but I'm not 100% on that.

[adamreisnz]

I see, interesting read. I guess their main concern is maintaining backwards compatibility with callbacks. It also looks like the discussion ended about a year ago.

[reconbot]

For me it's not backwards compatibility it's principal of least surprise. I'd like node serialport to work like everything else does.

[adamreisnz]

Someone needs to be the trend setter ;)

[Ks89]

Everything in NodeJs is moving to promises, so I think that a new major release non-backward compatible is acceptable.