Sensor emits null argument too much

[boneskull]

For most/all events emitted in Sensor, the first argument is the variable err, which seems to always be null. Remove it?

[dtex]

@boneskull Wow, you weren't kidding about err being null all the time. I bet there is some history to explain the existence of that param and it hasn't been removed because doing so could break user code. Let's see what @rwaldron says about it.

[BrianGenisio]

The "error-first callback" is pretty much the standard in Node. I agree that our emit() calls are inconsistent through the code base, but I believe we should be using "error-first" everywhere for consistency. Even if we always return null, it allows us to return an error in the future in a consistent and predictable way. If we were to make any change (and I think we should), I would vote to standardize throughout the codebase to use "error-first" and return null for now.

[rwaldron]

Patch welcome I guess.

[rwaldron]

Said patch needs to update all tests and examples.

[rwaldron]

I'm sure no one will like this, but I think the "error-first [cargo cult] convention" is a bad programming pattern. I started off doing that and overtime it was clear that there was nothing interesting to pass there, and abandoned it. For the sake of not breaking code, I left the old cases alone and just didn't follow the pattern in new APIs. I don't really care either way, someone's code will be broken but it needs to be addressed for the sake of consistency.

[BrianGenisio]

I'm sure no one will like this

I'm rather ambivalent about it as a programming pattern, so I wouldn't say that I don't like your opinion. Consistency within the project is more important than consistency with other Node projects IMO. If given my druthers, however, I tend to prefer consistency with the idioms of the environment, if only to play well with others. "Error-first" lets us interop with thunks and promises when we want to (I realize that thunks and promises don't always make sense in J5, but sometimes they do). They make our APIs more accessible to new developers, etc.

I do care enough to make it consistent. In my recent contributions, I had to make the seemingly arbitrary choice and a clear choice would have felt cleaner.. I'd be happy to make a consistency pass as long as we define what the consistent approach is. :)

[boneskull]

The "error-first callback" is pretty much the standard in Node. I agree that our emit() calls are inconsistent through the code base, but I believe we should be using "error-first" everywhere for consistency.

Events != async callbacks. I'd prefer error-first for callbacks, for consistency (and interoperability with libs like async and Q).

But I've never seen a pattern in any library where all events emit an err object and some other data. This seems bizarre. If you have an error, an error event should be emitted. Otherwise, emit the appropriate data, if anything.

(Note that if you do emit an error event and do not listen for it anywhere, I'm pretty sure Node will toss an Exception and your program will crash.)

[boneskull]

I'm sure no one will like this, but I think the "error-first [cargo cult] convention" is a bad programming pattern.

I agree, basically. But my real beef is with callbacks in general. I'm no NodeJS history buff, but Promises seemed to become popular because of the overuse of callbacks in NodeJS code.

In my experience, most asynchronous tasks can be handled more cleanly and elegantly with Promises (and the proper library). As @rwaldron mentioned in another ticket, the pattern has limitations, but generally I've found if you write code "Promise-first" you don't have many problems.

But unfortunately, converting a callback-based codebase to a Promise-based is daunting and won't result in a clean mapping. I'm not advocating that, mind you--just offering an opinion.

[BrianGenisio]

Events != async callbacks.

Right.

Note that if you do emit an error event and do not listen for it anywhere, I'm pretty sure Node will toss an Exception and your program will crash.

Yeah, that is right too. EventEmitter will do that.

Two great points. And the dos for EventEmitter corroborate what you are saying. You 've convinced me that on('data', function(data) {}) is the correct interface.

[rwaldron]

Let's...

• Fix all the emitters to remove the error-first mistakes. I'm ok with this breaking change, but of course all the tests need to be updated as well
• Fix any (I don't think there are many) callback handlers to include error-first arg lists

[BrianGenisio]

Sounds like a plan. @boneskull, were you planning on tackling this? If not, I'd be happy to, but I'm not likely to think about it for a couple of weeks. (still have lots to do for my NodeBots event on 1/6)

[boneskull]

@BrianGenisio Unlikely. I have a billion projects and time for none of them.

[Resseguie]

Question since I've not dealt with error-first pattern much.

In some cases, like in Led, we typically don't pass anything back in the callback.
Eg: https://github.com/rwaldron/johnny-five/blob/master/lib/led.js#L300-L302

But instead of just calling callback() should it explicitly call callback(null) just to be clearer that it's error-first?

[Resseguie]

Also, should examples like that in Led use callback.apply(this,...) (with some appropriate reference to the Led itself) as in other cases:
https://github.com/rwaldron/johnny-five/blob/master/lib/pin.js#L197

[Resseguie]

@rwaldron preferred name for callback functions? Looks like we have at least callback, cb, and callbackfn sprinkled around the code.

[dtex]

Do y'all feel that callbacks and listeners are the same thing? This thread doesn't seem to make a distinction between the two. I've always considered them to be different (listeners being bound to event emitters, callbacks being... well... callbacks that aren't listeners).

Aside: Even the node.js event emitter docs use both labels.

err first on callbacks is an established practice but it is not an established practice on event listeners. I think that's why err first feels so wrong here. Instead, event emitters should emit an "error" when appropriate and we should have listeners for that.

[BrianGenisio]

@dtex: @boneskull pointed this out earlier in the thread and I think we all agree that there is a distinction, and that "error-first" doesn't make sense for events, only for async callbacks. Per @rwaldron's request, someone (possibly me when time permits) will go through the code and fix EventEmitter.emit() code to not include err objects, but find any async callbacks (if there are any) and make sure they are following the "error-first" convention.

[dtex]

Ah, thank you. I overlooked that.

[rwaldron]

@Resseguie

• callback for callbacks
• handler for event handlers

Sorry for being inconsistent over the years and thank you all for helping identify these things so we can clean them up :)

Also, @boneskull: keep up the great deep digging!

[divanvisagie]

@rwaldron As action was taken and 2 new issues raised from this thread, I assume this issue can be closed.

[rwaldron]

Yeah, that's fine. I'm maintaining the patch until we commit to 0.9.0