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

Add types to node-sonos #512

Open
realwakils opened this issue May 5, 2021 · 7 comments · Fixed by #519
Open

Add types to node-sonos #512

realwakils opened this issue May 5, 2021 · 7 comments · Fixed by #519

Comments

@realwakils
Copy link

Hello, I'm adding this issue to raise awareness about the desire of not only me, but I'm sure for others, for node-sonos.

Tbh i'm not personally very familiar with adding types to packages, but I might work on it.

Anyways if anyone else wants to, you can refer to this issue

(NOTE: I can't label this as a 'enhancement' or 'feature request'?)

@svrooij
Copy link
Collaborator

svrooij commented Jun 19, 2021

Your question is understandable, but this library is build in JavaScript and not in typescript. Personally I have no experience on how to add types to an existing library. Is there a typescript command to do that?

apart from that, as a contributor to this library a few year ago I made a rewrite in Typescript. Maybe that solves your issue? https://github.com/svrooij/node-sonos-ts

I think @bencevans would probably accept a PR to add the typing file, if there is also a script to generate them from the current source.

@realwakils
Copy link
Author

Your question is understandable, but this library is build in JavaScript and not in typescript. Personally I have no experience on how to add types to an existing library. Is there a typescript command to do that?

apart from that, as a contributor to this library a few year ago I made a rewrite in Typescript. Maybe that solves your issue? https://github.com/svrooij/node-sonos-ts

I think @bencevans would probably accept a PR to add the typing file, if there is also a script to generate them from the current source.

Hi and thank you for your comment. Interesting library but perhaps it isn't updated and has the same features as this one? Anyways I definetly think that there should be types for this specific library anyways.

I did some research and found out that people add these types via DefinetlyTyped.

I have added SOME of the types but because I'm not too familiar with this library, I don't think I can move on further and also I have no idea how to actually run the tests for the types and so the experience of adding types with DefinetlyTyped has left me quite confused.

I thought that my types could potentially still be used though, so I have provided them down here. Beaware that they are not finished and might not even be correct. I hope some more familiar, maybe @bencevans, would want to finish/add to the types.

Thank you,
wakils

realwakils/DefinitelyTyped@7db80ca

@svrooij
Copy link
Collaborator

svrooij commented Jun 20, 2021

@realwakils you should check out the other library. It has at least all the features this library has.

My opinion about the types, it's better to have no types (like at the moment) then have incomplete/incorrect types, like you're proposing.

It should be possible to generate those, but creating them manually and not having them in the library is a bad idea, especially if they are incomplete. Then you get all those strange errors that you're expecting a method to return something but your types tell it it's just a void.

@realwakils
Copy link
Author

@realwakils you should check out the other library. It has at least all the features this library has.

My opinion about the types, it's better to have no types (like at the moment) then have incomplete/incorrect types, like you're proposing.

It should be possible to generate those, but creating them manually and not having them in the library is a bad idea, especially if they are incomplete. The. You get all those strange errors that you're expecting a method to return something but your types tell it it's just a void.

Cool I will check it out.
Yes I see your concerns in how the DefinetlyTyped library would have to follow along with this one or it could cause huge confusion. Personally though I think that adding types should be a large priority because for me (and I know for many others), Typescript speeds up my workflow alot and having to look at typings online in different docs is really just pain.

Anyways I will keep this issue open in case anyone wants to continue the discussion.

@svrooij
Copy link
Collaborator

svrooij commented Jun 20, 2021

One more question, what is the reason to put the types in a different repository? That makes it harder to manage.

This should be top priority

The maintainers make up the priority, you could also contribute these types and they will probably added to the package. Next to that there is a typescript library available, so the priority to add them to this library probably isn't very high.

But as said having incomplete/incorrect typings is even worse then no types. So this should be incorporated into maintaining this library.

@svrooij
Copy link
Collaborator

svrooij commented Sep 25, 2021

@realwakils I just found a typescript reference on how to generate types from JavaScript. It requires to add jsdoc comments to a lot of items.

https://www.typescriptlang.org/docs/handbook/declaration-files/dts-from-js.html

I would be happy to add all the scripting and the stuff in the package.json, but annotating all the variables will be a tedious task. I could definitely use some help with that.

svrooij added a commit to svrooij/node-sonos that referenced this issue Sep 29, 2021
svrooij added a commit to svrooij/node-sonos that referenced this issue Oct 3, 2021
@svrooij svrooij linked a pull request Oct 3, 2021 that will close this issue
@github-actions
Copy link

github-actions bot commented Oct 4, 2021

🎉 This issue has been resolved in version 1.15.0-alpha.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants