Use WebSocket events instead of callbacks - no API change

[gizmotronic]

I've been testing event handlers vs. callbacks using code structured in the same way that our WebSocket class is:

self.emit('something', data);
if (self.onsomething) {
    self.onsomething(data);
}

Before my testing I would have thought that callbacks were more efficient. They're simpler creatures without the overhead of yet another object. And, after all, the event handler is provided a callback to do the work, right? Handling events is significantly more efficient as it turns out. In some cases it's radically so.

I invite you to run my test script, make it a more accurate representation, and otherwise show how it doesn't say what I'm claiming. If you are seeing the same advantage, though, I think this is a worthy change without any impact to the API.

[samuri51]

how much faster is "remarkably so"?? like 1 ms? an ya it does have an impact, it requires me to change my bot code yet again. i think the perceived speed is more than adequate, and in reality that is all that matters. it doesn't seem worth it to find all callbacks in my 5000 line script and replace with event emitters for absolutely no human noticeable gain.

[samuri51]

if you wanted to make it backwards compatible though then i don't see any problem with this change... but it seems all of your changes force developers to change their code or have it be non-functional

[Izzmo]

@samuri51 that's what we have been talking about lately... backwards compatibility is key. If only for a short amount of time, it lets developers of this API take time to integrate into the changes. I've found usually it's 1 major-iteration when you delete the backwards compatibility. Just my 2 cents though.

[alaingilbert]

@samuri51 What code do you have to change ? These changes shouldn't affect your code. (I think !)

[gizmotronic]

1. There's no API change.
2. I'm not bumping the package version, but the required engine. The minimum required version of Node.js with this change is now v0.6. For all I know this is already true, but I have confirmed that my change forces the issue. You may notice that v0.6 is the unsupported release that's one older than the current maintenance release of Node.js, and two releases behind the current v0.10 stable release.

[Izzmo]

@gizmotronic Ah, I see. Well, I'm currently running 0.6, so that would be fine, and I'm pretty far behind. 0.4 is pretty old.

[ghost]

sounds like a good change, but will it affect the package version for the TTAPI.

[gizmotronic]

@alaingilbert - LOL yes - of course you're right. The perils of frequently switching between CoffeeScript and JavaScript!

I'm using this change in production on 2 different instances (4 bots total). Since onClose doesn't actually do anything it hasn't shown up in my testing. This begs the question as to whether we even ought to have the handler, but, if we're going to do something with it then my code needs to be corrected.

[gizmotronic]

@Izzmo right. Since OpenShift still defaults to v0.6 I feel that's still an important target to hit, in any event.

Heroku is quite a bit more progressive but I discovered a bit ago that they still support v0.4.7 and v0.4.10. I'm going to patch the request to allow the older engine.

[gizmotronic]

Request updated per discussion above.

[gizmotronic]

I've done testing on Node.js v0.4.10 and updated the gist with those results.

Since I've been running this for more than a week without incident, the compiled version has been added to the request.

There are no associated documentation updates as there's no impact to the API. This is strictly an internal performance enhancement.

[samuri51]

i tested this out and everything seems to work as expected so i'm giving it the :thumbsup:

[Izzmo]

So, what's the consensus on this, merge?

[gizmotronic]

I've run the code continuously without any problem since July.

@Izzmo, if you can confirm that it works for you too, then I'd be pretty confident that it's ready for production.

[gizmotronic]

Thanks @alaingilbert, much appreciated. I've bumped the version.

[alaingilbert]

I updated the npm package this morning :)