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

Typescript Types change from version 4.0.6 to version 4.0.7 #181

Closed
duccio opened this issue Jul 29, 2020 · 14 comments
Closed

Typescript Types change from version 4.0.6 to version 4.0.7 #181

duccio opened this issue Jul 29, 2020 · 14 comments

Comments

@duccio
Copy link

duccio commented Jul 29, 2020

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:

  • error TS2339: Property 'hue' does not exist on type 'States'.
  • error TS2339: Property 'sat' does not exist on type 'States'.
  • error TS2345: Argument of type 'BaseStates' is not assignable to parameter of type 'LightState'. Type 'BaseStates' is missing the following properties from type 'LightState': white, hsb, hsl, rgb, and 14 more.

Thanks!

diff

@peter-murray
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

duccio commented Jul 30, 2020

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
Copy link
Owner

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
Copy link
Author

duccio commented Jul 30, 2020

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
Copy link
Author

duccio commented Jul 30, 2020

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 [email protected]
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
Copy link
Author

duccio commented Jul 30, 2020

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.

@duccio duccio changed the title Typescript Types change from version 4.0.5 to version 4.0.7 Typescript Types change from version 4.0.6 to version 4.0.7 Jul 30, 2020
@peter-murray
Copy link
Owner

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
Copy link
Author

duccio commented Jul 30, 2020

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

> [email protected] 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
Copy link
Author

duccio commented Jul 30, 2020

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

> [email protected] 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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

duccio commented Jul 31, 2020

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
Copy link
Owner

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?

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

No branches or pull requests

2 participants