peter-murray / node-hue-api

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

Group hasn't `lights` property #157

Closed DenisPryt closed 4 years ago

DenisPryt commented 4 years ago

class Group check lights attribute, but don't hold it.

if (!this._attributes.lights) {
   throw new ApiError('Missing a valid lights attribute for the Group');
}

It's a normal behavior, or a bug?

DenisPryt commented 4 years ago

I think any type of group has a lights attribute. Maybe getter for lights should be in Group class?

peter-murray commented 4 years ago

Group is abstract, it has specializations, and only those are instantiable. The specializations have the lights attribute, which changes based on the type, e.g. room does not require any lights to be defined, where as a light group does.

How are you encountering this error?

DenisPryt commented 4 years ago

I do not consider this a mistake. This check

Just this code return array with abstract Groups. But abstract Group hasn't lights attribute. But all real classes has. I think it would be more convenient to provide get lights to group interface. For example

const groups = await hueApi.groups.getAll(); // Array<Groups> without .lights
peter-murray commented 4 years ago

It is a mistake, as I transitioned group to be abstract. The 4.x versions should not ever return a base group instance. That check is there to ensure that the instances implement the correct constraints on a group.

How are you managing to get this error, as the api should only ever return an implementation of one of the specializations.

DenisPryt commented 4 years ago

Sorry. I didn't get error. It works fine. Just model Group hasn't lights getter. But all derived classes have. And abstract Group "knows" about light attribute. I think that make get lights() in class Group is a good idea, despite the fact that all derived classes reimplement it. Because get lights() is a part of Group interface.

peter-murray commented 4 years ago

This is intentional, a user should never get one of these instances now, as the types from the bridge itself will be instantiated.

It is done this way due to the lights attribute being required in some groups and not others, but without a lot of hacking of the type system in use to support this one edge case, where I would like to toggle the required state on the attribute, it is just considerably easier to ensure that the concrete classes pass the attribute for the lights configured to what it wants.

This is in line with what I had to do in some of the sensor based objects as well.

DenisPryt commented 4 years ago

I understand. Thanks for fast answer.

peter-murray commented 4 years ago

Its the TypeScript definition that is not right here, I will update it.

Thanks.

peter-murray commented 4 years ago

Updated in 4.0.3