adding type definitions.

noodlefrenzy / node-amqp10

amqp10 is a promise-based, AMQP 1.0 compliant node.js client

MIT License

134 stars 56 forks source link

adding type definitions. #348

Open amarzavery opened 6 years ago

amarzavery commented 6 years ago

Porting over from https://github.com/typed-contrib/node-amqp10 which has lot of good stuff.
With this PR we will have type definitions shipped with the package itself.
This will make it easy to consume and make changes as and when required.

This change is

jvanharn commented 6 years ago

I would love this to be merged, as the "typings" client was deprecated in favour of @types npm packages, we no longer have a mechanism to automatically update typedefs.

Merging this PR would help greatly.

I would only exclude/revert the following files:

examples/simple_servicebus_queues.js -- The newline at the end should be there.
package-lock.json -- The package-lock is not currently included in the project, and shouldnt be included for libraries.
package.json -- Revert everything except the ""types": "./typings/lib/index.d.ts"," line; especially requireing typings for node 9.4 in the dependencies or higher is pretty bad.

If a (second) maintainer for this feature is required, then I would be glad to help out.

BorntraegerMarc commented 6 years ago

I would love to see types! But should we maybe create a PR over at https://github.com/DefinitelyTyped/DefinitelyTyped ? That's the official / most used repo to store types

ps2goat commented 6 years ago

no, it's recommended that libs carry their own typings with them, so they are kept up-to-date with the lib's functionality. DefinitelyTyped is for community contributions for packages they don't own.

BorntraegerMarc commented 6 years ago

@ps2goat do you have a source on that? Because I only heard / read that it should be published to DefinitelyTyped...

ps2goat commented 6 years ago

Reviewed 28 of 32 files at r1, 9 of 9 files at r2. Review status: all files reviewed, 2 unresolved discussions (waiting on @amarzavery)

typings/lib/policies/policy.d.ts, line 36 at r2 (raw file):

declare namespace Policy {
    /** Policy Configuration. */
    export interface Overrides {

This is where I had to pass in sasl configuration, but I don't see a property added in later commits. Would this be a good place for saslMechanism property? Or do you have it somewhere else? https://stackoverflow.com/a/43913433/2084315

typings/lib/policies/service_bus_policy.d.ts, line 4 at r2 (raw file):

// Project: https://github.com/noodlefrenzy/node-amqp10
// Definitions by: Maxime LUCE <https://github.com/SomaticIT/>
// Definitions: https://github.com/typed-contrib/node-amqp10

It's nice to give credit, but since this will evolve, and is in every file, should the credits be moved to the readme file? Somewhere visible and in a single place.

Comments from Reviewable

ps2goat commented 6 years ago

@BorntraegerMarc - that comment got lost in the reviewable.io publish lol.

I guess that this lib isn't written in TypeScript, so technically it could go on DT. But they call it out on the readme:

https://github.com/DefinitelyTyped/DefinitelyTyped

If you are the library author and your package is written in TypeScript, bundle the autogenerated declaration files in your package instead of publishing to DefinitelyTyped.