TypeScript issues - Githubissues

lukescott commented 6 years ago

I'm starting to use this library, and my code is in TypeScript. I've found a few issues. Some of these I can send a PR for, while others I need clarification for.

"types" key missing from package.json. TypeScript needs to know where the types file is. You can manually include it, but it would be nice if this was set. I can PR this.
Constructor requires settings, while code examples shows this as optional:
```
constructor(settings: any);
```
Changing from settings: to settings?: will fix this. Settings is also not typed. I can PR this.

(settings is optional, correct?)
setValue in the examples shows a 5 and 2 parameter version. TypeScript def only has 2 parameters. I could PR this with an overload.

setValue's 2nd argument has a Value interface of:

interface Value {
    value_id: string;
    node_id: number;
    class_id: number;
    type: string;
    genre: string;
    instance: number;
    index: number;
    label: string;
    units: string;
    help: string;
    read_only: boolean;
    write_only: boolean;
    min: number;
    max: number;
    is_polled: boolean;
}

This is where I'm confused. In the C++ wrapper the signature appears to be (roughly):

setValue(nodeid: number, class_id: number, instance: number, index: number, value: boolean|number)
setValue(valueId: ValueId, value: boolean|number)

The ValueId interface is:

interface ValueId {
    nodeid: number;
    class_id: number;
    instance: number;
    index: number;
}

I'm confused by Value because it is not clear where the value actually goes in the interface, nor does it appear in the wrapper (???). The wrapper counts the arguments and assumes that the value is argument index of 1 or 4. So I don't know what this Value interface is or how it is supposed to be used.

If I can get some clarification on this I can PR this and fix it.

setNodeOn and setNodeOff don't work and "are not guaranteed to work". From what I can tell looking at OpenZwave code it tries to set the value to 255 - assuming a multilevel switch. Yet the node-openzwave-shared docs seem to indicate it works for a binary switch... but it doesn't on mine. I also read somewhere that setNodeOn/setNodeOff is supposed to be deprecated?

Not necessarily TypeScript specific, but should these be removed? Deprecation comment possibly?

This is what I've found thus far. I may find more things as I actively work on this, and I can fix any issues.

lukescott commented 6 years ago

cc @rumata28 @ekarak from #201

rumata28 commented 6 years ago

I am not sure I could answer all the questions - it has been a while I worked with the lib. I wrote the typedef just to have any typedef, never had time to make it perfect. I do believe your PR would be welcomed.

I have nothing to say about 5th item, but 1-4 IMO all valid points. I feel especially embarrassed by 4th - just looked though - you are right. This looks just a my bug.

Sorry again - have no time to fix - I am on another project now. But I would be happy to know if somebody used and improved it.

Thank you for cc-ing me.

:)

lukescott commented 6 years ago

@rumata28 No problem. I can send a PR. I do need some clarification on 4 if possible. Looking at it further, I've made the following assumptions:

Value an ValueId are the same object, the ValueId interface has fewer properties. https://github.com/OpenZWave/node-openzwave-shared/blob/12edc98974ae31f77a49a408736adfbbf949c8f9/src/utils.cc#L224,L250 seems to support that.
nodeid in ValueId is a typo. It's supposed to be node_id. So far I can't find nodeid anywhere else besides C++ vars. node_id is used in the js code examples.
To fix this I can remove the current ValueId and rename Value to ValueId and fix the interfaces. Then I an made some of these properties optional with ?:.

Is this correct?

Can anyone confirm which properties are optional and which ones are required? Does it differ between methods?

blackshadev commented 5 years ago

Hi,

I ran into the same issue as you and began typing things. I will attach my file, maybe you can take some things from it. I did not yet turned it into a PR because it wasn't complete yet.

It has 2 files because I needed the enums imported in my code, and when declaring them you can't import them. I think how this would normally be done is adding these enums to the JS files such that you can declare and use them in the lib itself instead of importing it from somewhere else, but this works as well without me modifying the lib itself.

typings.zip

lukescott commented 5 years ago

Hey, sorry I should have posted an update on this, although for me I get to work on this a day every few weeks or so.

I discovered that the Value object should be used on notifications (the on methods that return value) and as I stated previously the ValueId object is used on setValue. I think it the C++ code both are called ValueId, but I haven't been able to dig further to find out.

So what you can do is create the following folder structure within your project:

types/
  node-openzwave-shared/
    index.d.ts

Then in your tsconfig:

{
    "compilerOptions": {
        "typeRoots": [
            "./node_modules/@types",
            "./types",
        ]
    }
}

I have attached the modified types file I have been working with.

index.d.ts.zip

When I have time I'll look thru your file and see I can combine them. Like you I haven't created a PR yet because I believe the types file is still wrong/incomplete. As I work on my project I keep finding stuff.

How it normally works is package.json defines the types file so it can be picked up by the TypeScript compiler. In fact, if the library does specify it you can't provide your own like I have. Another reason for me to wait to PR until I'm done as the merged PR would conflict with the ongoing process. Although technically I could PR an updated types file without updating package.json and leave that for a second PR.

If you are able to combine both and make a PR, feel free though.

blackshadev commented 5 years ago

Hi thanks, I did manage to get the types working, but you can't use the enums in your code because they are ambiant definitions. So in my code I can't use Zwave.Notification.Nop so yeah that is my major issue.

I will merge both files with as a starting point yours. I already notices your file is more complete on some points.

lukescott commented 5 years ago

With the types file I provided you should be able to do Zwave.Notification.Nop. Just add these to your tsconfig:

"allowSyntheticDefaultImports": true,
"esModuleInterop": true,

Then do:

import Zwave from "openzwave-shared"
Zwave.Notification.Nop

Tried it and the compiler isn't complaining in vscode.

You can also do this:

import Zwave, {Notification} from "openzwave-shared"
Notification.Nop

To accomplish it I had to wrap the other types in a namespace with how the code is structured. Normally the class would be a default export, but since the code is vanilla ES5 the export is set directly to the class. Since the namespace shares the same name as the class, TypeScript merges them together within the type space.

blackshadev commented 5 years ago

You sir, deserve a medal, thanks. Almost done with the merging. You missed some events and parameter definitions which I has, but yours ware more complete overall. back in a sec with the final version

blackshadev commented 5 years ago

Here you go. Works like a charm.

The only "issue" I have with the typings is that I like my interfaces to start with an "I" . (so IValue instead of Value) but yeah, but since that is just a minor issue I left it untouched for now.

openzwave-shared.d.zip

blackshadev commented 5 years ago

Oh and the ControllerStates I copied over from the openzwave website so the naming is not really consistent. So we can fix that before we turn this into a pull request.

Edit: I missed the typo I fixed in my version and my code started to complaint Notification.NodeDeal -> Notification.NodeDead

lukescott commented 5 years ago

Awesome! The only thing I noticed (which I forgot to fix) is setValue needs | string added for value: for enums. I'm also going to go over the C++ code for setValue a little more thoroughly later to determine if there are any overloads I missed - the original author had Value as an option, which at the time didn't seem to be supported.

blackshadev commented 5 years ago

Cool. Shall I set up a small repo to keep our changes in sync until we sent a PR to this repo?

lukescott commented 5 years ago

I think we can use a gist maybe. It has revision support:

https://gist.github.com/lukescott/29c968e85420cd33c17590634c6c6882

blackshadev commented 5 years ago

I did the same thing!
I will use yours ;) thx

blackshadev commented 5 years ago

I got an error when using the enums as you suggested

TypeError: Cannot read property 'MessageComplete' of undefined

/home/vincent/sense/dist/zwave/node.js:120
            case openzwave_shared_1.Notification.MessageComplete:

for the code

import { NodeInfo, Notification, Value, ValueId } from "openzwave-shared";
...
 public applyNotification(notif: Notification): void {
        switch (notif) {
            case Notification.MessageComplete:
            case Notification.Nop:
                this._lastAwake = Date.now();
                break;
...

lukescott commented 5 years ago

Hmmm. It think that if an enum is defined in a .ts file and not a .d.ts file code is generated for it. We may have to include JS code in the PR for it to work properly.

Something along the lines of:

var enums = {
  Notification: {
    MessageComplete = 0,
    Timeout = 1,
    Nop = 2,
    NodeAwake = 3,
    NodeSleep = 4,
    NodeDeal = 5,
    NodeAlive = 6,
  },
  // ...
})

var instance;
module.exports = Object.assign(function(options) {
  if (!instance) instance = new addonModule.Emitter(options);
  return instance;
}, enums)

VSCode doesn't show an error because it thinks the enums are already defined.

blackshadev commented 5 years ago

I could not modify your gist and to keep things going I created a small repo. If you have any additions, please let me know or submit a PR. One more note, when do we think that this is "done" and ready for a PR, because to be honest, what we had is already way better than the initial type definition file. Maybe expose the enums like you suggested, anything else?

https://github.com/blackshadev/openzwave-shared-ts

lukescott commented 5 years ago

setValue(valueId: ZWave.ValueId, value: boolean | number): void; is missing string |.

There will probably be a few more things, but it will probably be days / weeks before I get into it. So if you want you could PR it now and we can do a follow up later. Make sure to add the "types" key to package.json.

Unfortunately as soon as you add the "types" key, it will override the local definition. In order to continue modifying it you have to delete the types file within node_modules. If you npm install anything else, it will restore the file. Kind of a pain, but it is possible to continue.

The next thing I want to focus on is comparing Value to ValueId. I think in the C++ code there is no Value, but they are both ValueId. Possibly some of the values are optionals? Not sure. I have to dig into it and find out. And the setValue may have an additional overload that takes just Value. Have to read it more carefully and find out.

blackshadev commented 5 years ago

Check, I added the string type. I will see what I can do for the PR.

lukescott commented 5 years ago

Also if you do something like this:

var enums = {
  Notification: {
    MessageComplete = 0,
    Timeout = 1,
    Nop = 2,
    NodeAwake = 3,
    NodeSleep = 4,
    NodeDeal = 5,
    NodeAlive = 6,
  },
  // ...
})

var instance;
function init(options) {
  if (!instance) instance = new addonModule.Emitter(options);
  return instance;
}

module.exports = Object.assign(init, {default: init}, enums)

You should be able to use import without needing:

"allowSyntheticDefaultImports": true,
"esModuleInterop": true,

(As you are setting the default export. Test to make sure)

lukescott commented 5 years ago

@blackshadev I've updated:

setChangeVerified(valueId: ZWave.ValueId): void;

to:

setChangeVerified(valueId: ZWave.ValueId, enabled: boolean): void

This function tells OZW to wait to send a value changed event on certain dimmers that slowly ramp-up/down, like the WD100Plus.

blackshadev commented 5 years ago

Thanks,I added it to my branch, for which the merge request is still pending

https://github.com/OpenZWave/node-openzwave-shared/pull/263/commits/408650eab97432f491b9a07f47f2736aaabac18a

lukescott commented 5 years ago

Sorry I'm just stringing this along. I'm posting this as I find stuff while working on my project 😄.

Excldue: string;

Should be:

Exclude: string;

(Option is misspelled)

blackshadev commented 5 years ago

I added it to the repo, so when the merge request gets accepted.. some time in the distant feature, it will be in.

OpenZWave / node-openzwave-shared

TypeScript issues #243