Add missing types to client events, light properties, product details#45
Add missing types to client events, light properties, product details#45AdamCoulterOz wants to merge 4 commits intonode-lifx:mainfrom
Conversation
AdamCoulterOz
changed the title
ristomatti
left a comment
ristomatti
left a comment
There was a problem hiding this comment.
Nice improvements, thanks! I've added a few comments.
ristomatti
changed the title
|
I've renamed the PR to better reflect the changes. |
| import {EventEmitter} from 'eventemitter3'; | ||
| import {AddressInfo} from 'net'; | ||
|
|
||
| export class ClientOptions { |
There was a problem hiding this comment.
Thanks for defining these also but this shold definately be an interface as it's passed in as a plain object.
There was a problem hiding this comment.
Rationale for Utilizing ClientOptions as a Class
1. Encapsulation:
- Having
ClientOptionsas a class allows for the encapsulation of default values alongside the option attributes. - This encapsulation makes it straightforward to manage default values in a centralized location, thereby reducing the complexity in the
initmethod where these options are utilized.
2. Ease of Use:
- The class constructor can easily merge user-provided options with default values, providing a clean, intuitive interface for consumers.
- This also streamlines the code in the
initmethod, as it can assume that all necessary options are properly initialized.
3. Type Safety:
- The class structure ensures type safety for the configuration options while allowing for partial overriding of default values.
- This is beneficial for maintaining code integrity and reducing potential runtime errors.
export class ClientOptions {
/** The IPv4 address to bind to */
address: string = '0.0.0.0';
/** The port to bind to */
port: number = 0;
/** Show debug output */
debug: boolean = false;
// ... other properties with default values ...
constructor(options?: Partial<ClientOptions>) {
Object.assign(this, options);
}
}
const options = new ClientOptions({ address: '127.0.0.1', debug: true });| * @param {Function} [callback] Called after initialation | ||
| */ | ||
| init(options: any, callback: any): any; | ||
| /** Creates a new socket and starts discovery */ |
There was a problem hiding this comment.
Please revert this back. The JSDoc's are shown in IDE's when you hover over the function.
| * @param identifier label, id or ip to search for | ||
| */ | ||
| light(identifier: any): any; | ||
| light(identifier: string): Light | false; |
There was a problem hiding this comment.
Please revert the JSDoc as it was. It's simpler to have the same text here as in the code. The types in JSDocs are unnecessary though, so you're welcome to remove them if you wish.
| import {AddressInfo} from 'net'; | ||
|
|
||
| export class ClientOptions { | ||
| /** The IPv4 address to bind to */ |
There was a problem hiding this comment.
At least WebStorm doesn't seem to show these when I hover over client.init so these aren't much of use here. See my comments on the client interface.
There was a problem hiding this comment.
They should. It does in vscode.
| * @param callback called when light did receive message | ||
| */ | ||
| color(hue: any, saturation: any, brightness: any, kelvin: any, duration: any, callback: any): void; | ||
| color(hue: number, saturation: number, brightness: number, kelvin: number, duration: number, callback: (err: Error) => void): void; |
| * @param apply apply changes immediately or leave pending for next apply | ||
| * @param callback called when light did receive message | ||
| */ | ||
| colorZones(startIndex: number, endIndex: number, hue: number, saturation: number, brightness: number, kelvin: number, duration: number, apply: boolean, callback: (err: Error, msg: any, rinfo: RemoteInfo) => void): void; |
| saturation: number; | ||
| } | ||
|
|
||
| export class constants { |
There was a problem hiding this comment.
Revert this back to const as that's what it is. We're not instantiating it anywhere. This could also break someones code ( I think).
| export class Client { | ||
| import {RemoteInfo} from 'dgram'; | ||
| import {EventEmitter} from 'eventemitter3'; | ||
| import {AddressInfo} from 'net'; |
There was a problem hiding this comment.
If we're referencing types from other packages, I believe we should have them as dev dependencies in package.json also.
There was a problem hiding this comment.
Here's a clarification:
-
Core Node Modules (
dgramandnet):- These are included with Node.js, and their types are covered by the
@types/nodedev dependency.
- These are included with Node.js, and their types are covered by the
-
eventemitter3:- Agreed. I've added
eventemitter3as a dev dependency.
- Agreed. I've added
| ZONE_INDEX_MINIMUM_VALUE: number; | ||
| }; | ||
|
|
||
| export namespace packet { |
There was a problem hiding this comment.
I did not ask to remove this. I know some people use these to overcome the lack of some features on this library. Please revert.
| } | ||
|
|
||
| export namespace utils { | ||
| export interface utils { |
There was a problem hiding this comment.
Please revert this to namespace and as it where. I don't think it's a good idea to remove types we've previously published. I might have made a mention on private/public APIs but since the functions in utils had JSDoc's, they could be used by some.
|
Sorry about the amount of comments but there was a lot of unexpected changes. Some good, some not. At least for me autocomplete is quite broken now. Did you test these? Before:
So with the changes it would seem it's not possible to create a new client instance (unless I'm doing something wrong with my quick testing). |
2 participants
No description provided.