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

adding type definitions. #348

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

amarzavery
Copy link
Collaborator

@amarzavery amarzavery commented Jan 24, 2018

  • 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 Reviewable

@jvanharn
Copy link

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
Copy link

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
Copy link

ps2goat commented Jun 14, 2018

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
Copy link

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

@ps2goat
Copy link

ps2goat commented Jun 14, 2018


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
Copy link

ps2goat commented Jun 14, 2018

@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.

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.

4 participants