search

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

Typescript Types change from version 4.0.6 to version 4.0.7 #181

Closed duccio closed 4 years ago

duccio commented 4 years ago

Hello guys,

when updating from version 4.0.6 to version 4.0.7 I lost a lot of types in lib/model/lightstate/LightState.d.ts (attached a graphical diff).

Is changed something in type generator? I'm collecting a lot of error like this one:

Thanks!

diff
peter-murray commented 4 years ago

All those missing values come from the base class. I have not changed anything explicitly, but it always uses the latest version of the typescript node module when generating them so it is possible there is some substantial change in version that is being pulled down.

I will take a deeper look into this, thanks for reporting it.

peter-murray commented 4 years ago

I set up a sample TypeScript project that imports the latest version, 4.0.7 and I can see the types as they should be defined in a LightState object that I created.

The parts that are missing in the diff all come from the classes that LightState extends and I can see these in IntelliJ IDEA when using code completion.

Can you provide a link to your project or share more information like the version of TypeScript that you are using and some sample code that replicates your issues, as it is working as I would have expected under 3.9.7 of TypeScript in the project that I tested with.

duccio commented 4 years ago

Hello Peter, thanks for you reply! I just double checked and I have not updated the typescript version (still 3.8.3) and cannot find inside node_modules/node-hue-api a local typescript module.

Just tried in a clean repo:

And discovered that the problem is only with version 4.0.7. The 4.0.6 version works like expected, just to let you know...

My project is not public, sorry. I try soon to create a sample code/repo.

Thanks again.

peter-murray commented 4 years ago

The project is a JavaScript project and I just generate some TypeScript definition files at publish time to the npm registry.

I don't want TypeScript appearing in my dependencies (especially if it is only used to generate some type definition files), so I use npx to run the typescript type generation. I have not pinned that to a version (so it always gets the latest version) when it generates the types.

It is possible that something has changed in the various versions of the TypeScript version used at the time of generating the previous versions of the definitions.

Unfortunately due to just using the latest version I cannot easily back trace to what version of TypeScript was used when generating, as the code has not really changed in that area across the point releases for the project.

What IDE are you using? How are you seeing the errors? Granted the file has less definitions in the file, but it should pull the class definitions from the other classes that it extends from, which is the behaviour I am seeing in IntelliJ IDEA.

duccio commented 4 years ago

I see errors also not using IDE but only CLI... the errors come from the build of the project (npx tsc - version is 3.8.3 for hode-hue-api 4.0.6 and 4.0.7):

src/controllers/message/device/load.hue.message.ts:57:18 - error TS2339: Property 'hue' does not exist on type 'States'.

57       lightState.hue(this.lightState.hue)
                    ~~~

src/controllers/message/device/load.hue.message.ts:58:18 - error TS2339: Property 'sat' does not exist on type 'States'.

58       lightState.sat(this.lightState.sat)
                    ~~~

src/controllers/message/device/load.hue.message.ts:59:18 - error TS2339: Property 'bri' does not exist on type 'States'.

59       lightState.bri(this.lightState.bri)
                    ~~~

src/controllers/message/device/load.hue.message.ts:62:18 - error TS2339: Property 'sat' does not exist on type 'States'.

62       lightState.sat(config.hue.prop.hue.sat)
                    ~~~

src/controllers/message/device/load.hue.message.ts:64:16 - error TS2339: Property 'on' does not exist on type 'States'.

64     lightState.on(true).transitionDefault()
                  ~~
duccio commented 4 years ago

Update (to replicate the problem):

mkdir test-hue
cd test-hue
npm init
npm i node-hue-api
cat node_modules/node-hue-api/lib/model/lightstate/LightState.d.ts

export = LightState;
declare const LightState_base: typeof import("./CommonStates.js");
declare class LightState extends LightState_base {
    white(temp: any, bri: any): import("./LightState.js");
    hsb(hue: any, saturation: any, brightness: any): import("./LightState.js");
    hsl(hue: any, saturation: any, luminosity: any): import("./LightState.js");
    rgb(red: any, green: any, blue: any): import("./LightState.js");
}

Forcing 4.0.6 version instead:

mkdir test-hue
cd test-hue
npm init
npm i node-hue-api@4.0.6
cat node_modules/node-hue-api/lib/model/lightstate/LightState.d.ts

export = LightState;
declare const LightState_base: typeof import("./CommonStates.js");
declare class LightState extends LightState_base {
    white(temp: any, bri: any): import("./LightState.js");
    hsb(hue: any, saturation: any, brightness: any): import("./LightState.js");
    hsl(hue: any, saturation: any, luminosity: any): import("./LightState.js");
    rgb(red: any, green: any, blue: any): import("./LightState.js");
    alert(value: any): import("./LightState.js");
    bri_inc(inc: any): import("./LightState.js");
    sat_inc(inc: any): import("./LightState.js");
    hue_inc(inc: any): import("./LightState.js");
    ct_inc(inc: any): import("./LightState.js");
    xy_inc(x_inc: any, y_inc: any): import("./LightState.js");
    alertLong(): import("./LightState.js");
    alertShort(): import("./LightState.js");
    alertNone(): import("./LightState.js");
    on(on: any): import("./LightState.js");
    off(): import("./LightState.js");
    bri(value: any): import("./LightState.js");
    hue(value: any): import("./LightState.js");
    sat(value: any): import("./LightState.js");
    xy(x: any, y: any): import("./LightState.js");
    ct(value: any): import("./LightState.js");
    effect(value: any): import("./LightState.js");
    transitiontime(value: any): import("./LightState.js");
    brightness(value: any): import("./LightState.js");
    saturation(value: any): import("./LightState.js");
    effectColorLoop(): import("./LightState.js");
    effectNone(): import("./LightState.js");
    transition(value: any): import("./LightState.js");
    transitionSlow(): import("./LightState.js");
    transitionFast(): import("./LightState.js");
    transitionInstant(): import("./LightState.js");
    transitionInMillis(value: any): import("./LightState.js");
    transitionDefault(): import("./LightState.js");
    reset(): import("./LightState.js");
    populate(data: any): import("./LightState.js");
    _setStateValue(definitionName: any, value: any): import("./LightState.js");
}
duccio commented 4 years ago

I just discovered the problem is in the package loaded in the NPM registry:

https://registry.npmjs.org/node-hue-api/-/node-hue-api-4.0.7.tgz

If you download the row package and the old one (https://registry.npmjs.org/node-hue-api/-/node-hue-api-4.0.6.tgz) you'll find that the file lib/model/lightstate/LightState.d.ts is different and lost some types... hope this help.

peter-murray commented 4 years ago

I don't think that is right, at least in the context of this test project: https://github.com/peter-murray/test-node-hue-api-typescript

If you download that project, it uses the 4.0.7 version of the project and compiles using npm run build the TypeScript compiler works and does not complain.

Can you take a look at that project, as compare it or modify it to replicate your use case and errors?

duccio commented 4 years ago

Thanks for the test project.

This code:

import hue from 'node-hue-api';

const lightstate = new hue.v3.model.lightStates.LightState();
lightstate.transitionFast().hsb(128, 100, 100);

works fine in 4.0.6:

npm run build

no-output, all is ok

but doesn't work on 4.0.7:

npm run build

> testTypeScript@1.0.0 build test-node-hue-api-typescript
> tsc

index.ts:4:29 - error TS2339: Property 'hsb' does not exist on type 'BaseStates'.

4 lightstate.transitionFast().hsb(128, 100, 100);
                              ~~~

Found 1 error.
duccio commented 4 years ago

Another example, this code:

import hue from 'node-hue-api';

const lightstate = new hue.v3.model.lightStates.LightState();
lightstate.populate({}).hue(128);

works fine in 4.0.6 but doesn't work on 4.0.7:

npm run build

> testTypeScript@1.0.0 build test-node-hue-api-typescript
> tsc

index.ts:4:25 - error TS2339: Property 'hue' does not exist on type 'States'.

4 lightstate.populate({}).hue(128);
                          ~~~

Found 1 error.
peter-murray commented 4 years ago

Ah, that is certainly the information I was needing here... So it is the fact that base classes return only instances of the base class, hence the chaining end up breaking because of this.

I will have to look deeper into what is going on with respect to the TypeScript definition generation as this is just driven from the JSDoc.

Thanks for the clarification.

peter-murray commented 4 years ago

I managed to finally trace this through and get to the bottom of it. It was a couple of things, the removal of the existing TypeScript definition files was not operating correctly, so the definitions from previous releases could be picked up.

Once I understood what was happening there, I was able to focus in on the regeneration of the definition files from the JavaScript and it appears to be related to some changes in the 3.9.x version of TypeScript, so I have now locked to the 3.8.3 version that last worked in correctly generating these definition files.

I have released version 4.0.8 to the npm registry, please try using that version as the definition files should now be correct.

duccio commented 4 years ago

Hello Peter, thanks.

Now it works, but there is another problem about types.

In the last definition (before 4.0.7) (from lib/model/Light.d.ts) we have:

get state(): any;
get capabilities(): any;

in the new definition:

get state(): Object;
get capabilities(): Object;

any (in this use case and IMHO) is better and easy to manage.

Thanks!

peter-murray commented 4 years ago

These have been set as Objects in the JSDoc for sometime now, and they are actual objects with keys and properties. The data payloads change depending upon the instance of the light.

Why is this being an Object causing issues in this case?