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 type definitions #296

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

Conversation

nfantone
Copy link
Contributor

@nfantone nfantone commented Nov 2, 2016

@squaremo I don't know if you are really into this - but hey, here it is, nonetheless.

This PR would allow library/module writers to consume "official" type definitions for amqplib in their TS 2.0 projects (such as rx-amqplib) without hassle, by just installing the client from npm.

npm i amqplib
npm i --save-dev @types/bluebird @types/node

Additional "peerDependencies" are needed because the definitions file (index.d.ts) depends on external typings. Regular users won't need to install those and can safely ignore all this.

@squaremo
Copy link
Collaborator

squaremo commented Feb 8, 2017

This looks worthwhile. I am a little worried that it means people changing the JS code will need to also change the type definitions, or they will be out of date (i.e., it will require some expertise in TypeScript). Can you comment on that?

@@ -13,18 +13,25 @@
},
"dependencies": {
"bitsyntax": "~0.0.4",
"bluebird": "^3.4.6",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the shuffling here. Seems like npm i --save sorted entries alphabetically.

@nfantone
Copy link
Contributor Author

nfantone commented Feb 8, 2017

@squaremo Well... the answer is: it depends. If changes on JS code update the API, TS definitions should be changed as well to reflect that. No way around that, I'm afraid.

it will require some expertise in TypeScript

Typescript is very easy to grasp if you are familiar with ES6+. And we are only interested in typings here. Just by looking at the only relevant file, index.d.ts, you can already get a feel of what's going on. No bones about it, really.

If this is a concern for you, people can always install those using typings via DefinitelyTyped repos. Microsoft is trying to move users away from that, though.

Your call.

@lautarodragan
Copy link

+1
This would add a lot of value to us TS users.

The "ideal" way proposed by the TS team is for projects to offer their own typings. This gives them more visibility and helps to keep them updated, since the typings would be "official".

The alternative is having them in the DefinitelyTyped repo (the url changed) and publishing them to @types/amqplib, installing them with npm i -D @types/ampqlib.

As @nfantone said, the typings would only need to be updated if the API changes.

@kibertoad
Copy link
Collaborator

@squaremo What is the final decision on that, do we want it in? I can fix the conflict in this PR if it is OK in general.

@MrManny
Copy link

MrManny commented Jul 23, 2019

It has been over a year. Any progress on this?

@squaremo
Copy link
Collaborator

squaremo commented Jul 26, 2019

Having used TypeScript in another context, I have a better appreciation of why type definitions are useful. Can we check the types are correct in the CI build?

@lautarodragan
Copy link

@squaremo you can't check that the types match the implementation if the source code isn't written in TypeScript itself, but there are some basic tests that can be done.

Basically declaring stuff and seeing it pass the type checker.

Here's ReactJS type definitions tests, for example: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/v15/test/index.ts

@Recodify
Copy link

Any chance we can get this merged?

@zernie
Copy link

zernie commented Jun 4, 2020

@squaremo, @nfantone sorry, have this been abandoned? Do you need some help?

@daniel-pedersen
Copy link

There is @types/amqplib. This has worked for our purposes. But official support would be appreciated. I can also help out if needed.

Base automatically changed from master to main February 17, 2021 22:01
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.

8 participants