peter-murray / node-hue-api

Node.js Library for interacting with the Philips Hue Bridge and Lights
Apache License 2.0
1.19k stars 145 forks source link

API cleanliness #86

Closed hansmbakker closed 5 years ago

hansmbakker commented 8 years ago

While creating Typescript typings for the node-hue-api module, I needed to get a close look at the API of the node-hue-api module.

Personally I found that the api could be made more clear. Of course the following is partly depending on taste, but:

I'd like to hear what you think. I think this is an opportunity to improve the quality of node-hue-api.

peter-murray commented 8 years ago

Hi,

Sorry for the slow response, I have been busy with work commitments.

The API has grown over the years, it was originally created before there was even an official API to wrap around. In some places it is starting to show its' age.

There are a lot of aliases as I find this useful from an API that can be used in many ways. I tend to favour fluent language like APIs, but there are others that do not. I tried to cater for both types here by using names which are close to that of the underlying API as well as variants around spelling (like colour and color) and in some cases more natural language. This really does not introduce must "clutter" as if you are going to use the library, you will probably only choose those that feel more natural.

Almost all the exposed APIs can be used as both a promise and callback. Node.js has always used callbacks (and still does for most of its' modules), whereas promises were an add on to the language. Of course there was some nice changes to introduce promises in Node.js 4.2.x. I have always used promises for most of my work in Node.js, but I have a user base that use mostly callbacks (at least almost all of those that post issues). I also have a number of users still on 0.10.x.

It is not hard to generate a more "clean" API that wraps the existing functions of the API for Typescript, but I have little experience of Typescript, and honestly I don't much like it, as I am a bit of a purist with Node.js. I can see the benefits that may attract some people to it, but I personally don't like to have to compile my javascript.

Would a "cleaner" API that I can expose for you be desirable and achieve what you are looking for? If so let me know and we can discuss what further changes you would find desirable.

Thanks

hansmbakker commented 8 years ago

I see that you've done a lot of effort on your library and I value that a lot, but personally I do not agree with your views on supporting several styles to do the same thing and on backwards compatibility with Node 0.10 that was released in 2013.

As for supporting lots of aliases and ways to code: I agree that it might not create clutter in the user's code, but it DOES create clutter in your own library. In my opinion this is not a good practice w.r.t. code quality and maintainability. Also it does not help new users in understanding how it works. Finally - allowing a user to write fluent code does not require the library to offer X different ways to do that.

As for supporting old styles of coding (callbacks): personally I think there are very few valid reasons to stick to such an old and soon unmaintained version of Node.

I would say that users who still use 0.10 can remain using an old version of this library. For the sake of people who keep up with new insights I would favor drawing a line, being brave and release a new version of the package that does have breaking changes, supporting a modern code style and a clear API definition.

Whether or not you use Typescript - the javascript version also deserves a good code quality.

Finally I think it is more important to keep up with the changes in the Philips Hue API than maintaining lots of ways to do things and to keep backwards compatibility.

pixelass commented 7 years ago

I agree with @wind-rider .

There is no reason to support node < 4.x IMHO. Things are moving a lot faster since last year. Legacy node users should be fine with the current version.

I also think it would be a lot easier to use one method/function per api-endpoint (drop the aliases)

The user should not need to worry about the names but be able to simply look them up in a dictionary, which then further describes the history/stats of that api-endpoint.

I'm also no fan of the q promises.

I wrapped all methods in pure functions with promises to make it more intuitive (mixed with other promise based actions it was confusing, when to use fail & done and when to catch)

const nupnpSearch = () => new Promise ((resolve, reject) => {
  hue.nupnpSearch().then(resolve).fail(reject).done()
})

I'm thinking about writing a new api, depending on the status of this repo

peter-murray commented 7 years ago

Due to the age of this issue now, most of my objections are getting close to being no longer valid, especially the ones about backwards compatibility for versions < 4.x.

Q is really showing it's age, and there is nothing that I am doing that cannot be handled by proper native promises now.

There is not a 1:1 correlation of my API functions to the Phillips Hue Bridge API ones. In some cases I provided functions that the bridge API did not initially provide, which did result in some of the deviations that I have, but in other cases I chose different names due to the obscure names that the underlying API provides. This made the API a little more user intuitive and corresponded with what a novice user may want to do rather than forcing them to understand all the concepts that Phillips uses in the Hue ecosystem.

If I am going to go down a path of changing the API functions to remove Q promises, then I would also remove the callbacks, and at that point would address some of the names of the functions. I still think it useful though in some cases to provide multiple aliases to some functions, especially when you are chaining function calls.

As I stated in #96 I will not have time to complete a major rewrite until November or later (but it would be this year if I do so).

rodrigoelp commented 6 years ago

Hi, just dropping in and I wanted to know if there has been any update since 2016. What is the current status?

peter-murray commented 6 years ago

I have about half of the rewrite done, but the APIs have changed a little in the meantime and I have to bring them in to line, and fill out the missing pieces. Work has been getting in the way and it will take some time to complete the refactoring. I have no definitive date currently as to when I will be able to release it in working form.

peter-murray commented 5 years ago

There is an alpha release 3.0.0-alpha.2 in the npm registry as of now, this will be released as the next major version once I complete the documentation. All the API functionality is practically locked down now for

peter-murray commented 5 years ago

3.0.0 was released today to npm registry that includes the new v3 api