-
Notifications
You must be signed in to change notification settings - Fork 37
Add typescript definition file for public API #52
Conversation
|
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
=======================================
Coverage 94.26% 94.26%
=======================================
Files 4 4
Lines 122 122
=======================================
Hits 115 115
Misses 7 7 Continue to review full report at Codecov.
|
Using "types" in package.json as suggested by the TS team http://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html. I followed DefinitelyTyped's https://github.com/DefinitelyTyped/DefinitelyTyped project structure. As a type test, I'm using the first example from the readme. |
eeb33d9
to
1712c92
Compare
1712c92
to
d7e43f2
Compare
Is something preventing this from being merged? |
😮 i'm sorry i missed this PR earlier! thank you so much for taking the time to add some typing. i'll review and give you some feedback now. |
i think this is a great start on getting types into this package. i'd like to also start thinking about whether we can leverage the AsyncAPI Spec to generate some of the types, so that re-syncing with the platform when additions or modifications are made becomes trivial. some of these ideas are also discussed here: slackapi/node-slack-sdk#496 |
@@ -0,0 +1,22 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its an interesting choice to essentially deliver the types in a separate package. i'm not certain what the advantages are to this approach over simply adding an index.d.ts
to the root of the package. can you explain? one disadvantage i see is that its common practice to list the @types/*
dependencies in the top-level package.json
, see this section of the manual.
is the intention that we would publish two separate modules to npm? if so, i think that would be confusing, and at that point we might as well only offer ambient declarations via DefinitelyTyped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typescript's publishing documentation suggests publishing to the @types organization on npm if the package is not written in TypeScript.
I believe the intent is to avoid shipping type definitions to those who don't need them.
That said, I'm happy to instead add index.d.ts
if that is your preferred approach.
"SLACKEVENTMIDDLEWARE_TOKEN_VERIFICATION_FAILURE"; | ||
|
||
type EventType = | ||
"app_uninstalled" | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have some apprehension to committing an exhaustive list of event types into this repo. these values are bound to change over time, and that means developers would have to deal with a compiler error if an up to date set is not available. we came up with a technique to allow flexibility using generics in this issue. i'd prefer a solution that allowed unknown event types to work without a compiler error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, if that's a goal then we could union String at the end or simply define EventType as a string.
Fixes #35.
Summary
Describe the goal of this PR. Mention any related Issue numbers.
Requirements (place an
x
in each[ ]
)