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

Better types for Scripting API #489

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

relu91
Copy link
Member

@relu91 relu91 commented Sep 25, 2023

This PR can be considered editorial in the sense that improves artifacts of the main note document. In typescript, Generics can be used to improve the description of a certain API. Here I add generic parameters to ExposedThing and ConsumeThing in order to capture compile-time errors like reading a property that does not exist in the consumed Thing Description or attaching a handler for an action that was not defined in the ExposeThingInit. The downside is that it makes the types a little bit more complex.

…to TD

Now ExposedThing and ConsumeThing types are generic and allows as parameters
of the different functions only the properties that are defined in the TD.
@danielpeintner
Copy link
Contributor

Looks very promising 👍

I tried to copy the update to a local node-wot instance and run into some issues..
image

I simply replaced index.d.ts with the updated version. Am I supposed to do more?

@relu91
Copy link
Member Author

relu91 commented Sep 25, 2023

Thank you for testing @danielpeintner ! Can you give me a little bit more context, thing is a ConsumedThing that is using which TD?

@danielpeintner
Copy link
Contributor

Can you give me a little bit more context, thing is a ConsumedThing that is using which TD?

I simply manually updated index.d.ts in the node-wot repo and looked at the counter-client sample which gave me all these warnings..

@relu91
Copy link
Member Author

relu91 commented Sep 25, 2023

Ah I see, then the problem is here:

const thing = await WoT.consume(td as ThingDescription);

Casting to plain ThingDescription type will cause keyof T.properties to return never 😢 . I don't if there is easy why to fix this without changing the example code 🤔

@danielpeintner
Copy link
Contributor

I see. The problem is that it is shown as an "error" and I don't think this is very good either from developers point of view.
Mhh, not sure how to move on from here...

@relu91
Copy link
Member Author

relu91 commented Sep 26, 2023

One option is to default to string instead of never. This allows people to cast variables to ThingDescription type and use readProperty and the other functions as before. If they use the correct typing (i.e., do not cast to ThingDescription) they get the type guards about names. However, this solution does not understand the difference between a TD without any property and a TD that has been cast to ThingDescription.

My preferred solution is to avoid casting to ThingDescription and let the compiler do its work. However, it seems that the ThingDescription type is a little bit too restrictive and as in the counter example, is not easy to create an already good TD using inline object literals 😢 .

@danielpeintner
Copy link
Contributor

Scripting API call Nov 20

  • we should apply this feature/improvement only if we are sure we don't cause any issues like mentioned above
  • --> wait for better TS type system or new ideas how to properly fix the issue

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

Successfully merging this pull request may close these issues.

2 participants