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 typescript declarations #8

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

Conversation

jabooth
Copy link

@jabooth jabooth commented Dec 12, 2016

Hi @alanhoff,

I'm using your library with Typescript, so I thought I would take a bash at writing a Typescript declaration file for portastic.

If you'd like, you could take this PR, and future TS users of portastic (who just npm install portastic as normal) will get to use the typing information provided here without any extra effort. Some tools/IDE's (like VSCode) will even provide typing assistance to normal JS users of the library based on this definition file.

Of course, the downside is that ideally the index.d.ts declaration needs to be kept up to date as you change your library, otherwise you will have some very confused TS users ;). I hope you feel the maintenance burden is acceptable - even if you don't know TS at all I imagine you can glance over the definition and not be too surprised about what is going on!

If that sounds like too much trouble, I can submit these definitions to DefinitelyTyped, which is a seperate repository of typing information. TS users will still be able to get types by doing:

npm install @types/portastic

The beauty of taking this PR instead is that the types can be versioned and kept in sync with the project.

You can read more about this whole thing from the official TS guide here.

Cheers!

@coveralls
Copy link

coveralls commented Dec 12, 2016

Coverage Status

Coverage remained the same at 86.555% when pulling adeaa6a on jabooth:ts-declaration into d27c7d4 on alanhoff:master.

@@ -28,6 +29,7 @@
},
"homepage": "https://github.com/alanhoff/node-portastic#readme",
"dependencies": {
"@types/node": "0.0.2",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that you expose an object that subclasses EventEmitter from node - a consumer of your API therefore needs the node typing information to fully understand your public API, hence the addition of the node types as dependency and not a devDependency (see Packaging dependent declarations)

IchHabRecht added a commit to IchHabRecht/node-portastic that referenced this pull request Mar 31, 2022
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