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

Request Cache implementation #17

Closed
wants to merge 1 commit into from

Conversation

fedhat
Copy link

@fedhat fedhat commented Nov 16, 2016

  • In order to address network errors, implemented a request cache that limits open http requests to a configurable maximum (defaults to 5).
    • This eliminates network concurrency errors (such as ECONNRESET) on 1st Gen Hue bridges.
  • Added new configuration property maxopenrequests and added an explanation of the property in README.md.
  • The "ECONNRESET" handling in 0.0.12 was removed, since the request cache prevents those errors from occurring.
  • Added deferred node module needed to create deferred promises for the request cache.
  • Updated version number to 0.0.13.

…um (defaults to 5). This eliminates network concurrency errors (such as ECONNRESET) on 1st Gen Hue bridges.
@ebaauw
Copy link
Owner

ebaauw commented Nov 16, 2016

Thanks, Federico @fedhat

I want to take a good look at your request, but that will have to wait until this weekend. On first impression your solution seems a bit more elaborate than I anticipated. Maybe I'm just intimidated by the use of deferred, which I'm unfamiliar with. Also I'm new to GitHub and haven't done any pull request yet.

@fedhat
Copy link
Author

fedhat commented Nov 16, 2016

@ebaauw I hope you find the PR useful. My goal was to write in your coding style, but no worries if it doesn't suit your needs. Also, feel free to ask me to make adjustments or answer questions regarding the code. Likewise, you can make changes yourself.

The code changes might look more elaborate than they actually are. I essentially moved what you had in HueBridge.request() into HueBridge.runCache(), along with some wrapper code to handle limiting requests to 5 open at one time. The changes in other parts of HueBridge were to add semicolons because my linter complains (rightly so) about them missing.

I first used promises in Angular JS, which uses the deferred style and loved them. When I switched over to ES6 / TypeScript development on a later project, I felt restricted with non-deferred promises. The great thing about deferred promises (in my opinion) is that you don't have to supply the code which handles calling either resolve or reject immediately. Not everyone likes deferred promises, however. I've seen them described as anti-patterns in some blogs. It might be a case of what a developer is used to or what he/she was exposed to first.

What deferred does is use a technique you might have seen where references to the resolve and reject functions are saved for later use:

var deferredPromise = new Promise(function(resolve, reject) {
    this.resolver = resolve;
    this.rejector = reject;
});

@ebaauw
Copy link
Owner

ebaauw commented Nov 19, 2016

Hi Federico @fedhat,

Indeed, I find this PR very useful; it teaches me a lot.

deferred is starting to make a lot of sense to me. Looking at the documentation, it's far more functionally rich than the Javascript native Promise. Did you have a look at deferred.gate()? To me it would seem that it already provides the functionality we need, without having to implement a request cache at all.

I started homebridge-hue (and my other, yet to be published, homebridge plug-ins) in the Node.js callback style, used by homebridge itself and by almost every other plug-in. Of course, I quickly ran into several Pyramid of Doom scenarios, so I decided to try and switch to promises. Intimidated by the number of competing implementations, and minimising the number of dependencies, I started out with the native Promise. Now, I think deferred would be a better choice, as it provides much added value.

  • deferred.promisify() to wrap callback style library functions (I was already defining my own wrapper function for this...);
  • deferred.gate(), as mentioned above;
  • deferred.map() for working with sets of promises, to replace the callback-style HueBridge.mapLights(), HueBridge.mapGroups(), etc.;
  • I'm already passing the resolver from HueBridge.createUser() to HueBridge.pressLinkButton() in an ugly callback style.

I'm not particularly interested in disputes on patterns/anti-patterns, but I am a big fan of consistency. So I would want to use deferred throughout my code, and not mix promise, deferred promise, and callback styles.

@ebaauw
Copy link
Owner

ebaauw commented Nov 19, 2016

Hi Federico @fedhat

Thanks for your remark on the semi columns! I'm relatively new to JavaScript and still learning the subtleties of the language. I grew up programming in the 1980s in C (even before C++ was created) and, until recently, still used vi as my main "IDE". My "coding style" (I suppose that's how you would call it) stems from that time. I'm no fan of JavaScript, nor, for that matter, of any weakly typed language (I must have a weak personality ;-) and I particularly dislike the fact that even syntax errors only reveal themselves at runtime. I do think Node.js is brilliant, however, despite my reservations for the JavaScript language.

I recently switched to Atom (which I found through GitHub Desktop), but still lack a proper IDE for JavaScript. What IDE and/or linter would you recommend? I'd like to run all my code through this linter.

@fedhat
Copy link
Author

fedhat commented Nov 19, 2016

Hi Erik @ebaauw

This kind of collaboration is the main thing I truly enjoy about working on open source software. :-D

I was laughing at myself when you asked about deferred.gate() because I completely missed that functionality. When I was working on my cache solution, I knew that I wanted a deferred promise implementation and picked deferred because it had very good documentation and was active. After installing it, I just forged ahead writing my cache without checking if there was anything else in deferred that would be of use. As you pointed out, deferred.gate() seems to be exactly what the concurrency issue calls for -- it will greatly simplify the solution I wrote.

I'm in total agreement with not wanting to get into disputes about the "right" way to do something -- consistency is much more useful.

@fedhat
Copy link
Author

fedhat commented Nov 19, 2016

@ebaauw Regarding JS linting, you're quite welcome. While I've been writing Javascript since the early days (circa 1997), I've never been a big fan of it, though I've had to accept that it has become the lingua franca of the internet. My first programming experiences were with BASIC in the 80's, then early Javascript, and then Java, Objective-C. Javascript's lack of types and runtime-only error checking is what got me interested in TypeScript. TS still transpiles to Javascript, but gives you a lot of what traditional programmers look for in a language (typing, abstraction, class definitions, private/public/protected properties and functions, interfaces, compile-time error checking). Also agree on your assessment of Node.js.

I spent more than a decade using emacs as my text editor of choice, but eventually switched over to IDEs. Currently my IDE of choice for Javascript development is WebStorm. It's a commercial editor with a lot of useful tools and functionality that I haven't found in open source editors. That said, Atom has lots and lots of great plugins.

The JS linter I use is JSHint. There's an Atom package for it: https://atom.io/packages/atom-jshint

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

Successfully merging this pull request may close these issues.

None yet

2 participants