Add ambient TypeScript definitions

[bmajz]

• I've read and understood the Contributing guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.
• I've been mindful about doing atomic commits, adding documentation to my changes, not refactoring too much.
• I've a descriptive title and added any useful information for the reviewer. Where appropriate, I've attached a screenshot and/or screencast (gif preferrably).
• I've written tests to cover the new code and functionality included in this PR.
• I've read, agree to, and signed the Contributor License Agreement (CLA).

PR Summary

This is the initial commit for ambient Typescript definitions for the node-slack-sdk project.

Some highlights:

• Provides a definition for all top level exports
• Provides a function definition for all top level Web Facets and RTM events
• Respects both promises and callbacks where applicable
• Follows the Typescript convention of having a root level d.ts with the same name as the js main/entrypoint

Work to be done:

• Still a lot of any and/or Object types, particularly around RTM events, Web API parameters and Datastore methods
• Some return objects can be further unified into tighter definitions and used across the definitons

Open questions:

• Break up into multiple .d.ts files?

In summary: there is more work to be done, but this should be a helpful starting point for the community.

Related Issues

Issue # 314

Test strategy

N/A. No impact. Ensured that definitions match usage of examples.

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #329 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #329   +/-   ##
=======================================
  Coverage   88.97%   88.97%           
=======================================
  Files          44       44           
  Lines        1234     1234           
  Branches      183      183           
=======================================
  Hits         1098     1098           
  Misses        136      136

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cecf343...7efd3d8. Read the comment docs.

[philcockfield]

+1 Please could we have this merged!!!

[ashubham]

@bmajz While we wait for the slackers to review this. It would be beneficial to the community if you submit this to DefinitelyTyped and/or roll out a @types/@slack/client package on npm ?

Although, there are some issues with this typescript definition files. For e.g. required params following optional params, some typings are wrong for example SlackChatPostMessageParams should not have channel but its a required prop in that interface. You might want to resolve these.

[bmajz]

hey @ashubham, I think it would be incumbent to then maintain those definitions and it seems like a pain to do that in two places for what should hopefully be a quick review. I also need to do an update to these defs, hopefully later this week to clean them up some more

[aoberoi]

sorry, we've had some offline discussions about this PR and i could have been more transparent about where we are with this. here's my recap. opinions and discussion welcome.

after talking to @bmajz, he asked that i hold off merging this PR because he'd like to improve it. he has some changes in mind, but i'll let him fill us in here if he can.

i also commented that im not sure going down this path is the right thing to do for our typescript community. there's no way we (as maintainers) can be certain that the definitions aren't broken because they are never tested against the actual library. we need something to gate us from making a change that breaks the interface definition because, in my opinon, wrong defs would be worse than no defs at all (anys are better than wrong types). and so, i'd like to discuss the pro's and con's of authoring the entire SDK in .ts files. this means the typescript compiler or linter will barf if we do anything inconsistent with our types. we could do this incrementally, but we have to be prepared to add yet another build step.

[bmajz]

Yes, I do need to push updates since there were some broken scenarios in there. Unfortunately, the team has been short handed recently and we haven't been able to circle back quickly. My sincere apologies.

However, I do agree with @aoberoi in that bad TS defn's can be worse than none at all. Ideally there isn't an extra translation step and the code matches the types. As a third party writing type definitions, I almost think it would've been less work to convert the code natively. I would enthusiastically support the conversion given the rest of the great work Slack is doing there:
https://slack.engineering/typescript-at-slack-a81307fa288d

[philcockfield]

I'm with you both on this. Massive and emphatic support for converting directly to .ts. After reading the "TypeScript at Slack" medium post a while back (that you also referenced @bmajz) I figured the SDK would be part of all that.

https://slack.engineering/typescript-at-slack-a81307fa288d

TypeScript really shines for SDK's and third party libraries, so given the internal move Slack is making toward TypeScript, having the outward facing SDK with Typescript generated .d.ts files makes a ton of sense.

Viva la Slack.

[bmajz]

Not sure why this caused a test to fail, should pass on retry by the looks of it

[aoberoi]

The next major version of this module will be authored in TypeScript. Until that version is out, I'm going to leave this PR open and actively point people to it when they ask for TypeScript definitions.

I feel like this is the best approach in the short term because it should signal to consumers that these are not maintained in a testable way, and if you choose to pull down the definitions, you're taking responsibility for them in your own app.

[philcockfield]

This sounds smart. Super glad to hear your re-authoring in typescript itself. Solid! 💪

[bmajz]

How does everyone feel about pushing the types into DefinitelyTyped/@types in the meantime?

[DominikPalo]

@bmajz I think it is not possible yet, because @types still can not support types for scoped packages (and the slack client module is scoped under @slack). See: microsoft/types-publisher#155

[bilby91]

Has anyone actually used this typings ? I'm having some trouble to use them. Not sure if is the problem with the scoped modules.

[bilby91]

I managed to get the typings working. I had to include the ws references and import them inside.

// Type definitions for node-slack-sdk
// Project: https://github.com/slackapi/node-slack-sdk
// Definitions by: Bilal Aijazi <https://github.com/bmajz>


/// <reference types="ws" />

declare module '@slack/client' {
  import * as WebSocket from 'ws'
  ....

BTW. I think that the scope packages issue is resolved. If I understood correctly this package need to be named slack__client

[bmajz]

I'll pull this change into my branch since I think that's where people will get it in the future. Will also look into what it takes to get this into DefinitelyTyped

[bilby91]

It would be awesome to have it DefinitelyTyped! Typings are working perfectly

[bilby91]

Any progress on this ? I had made some improvements to the typings provided in the thread but it's complicated to contribute if we don't have them in the @types repo.

[aoberoi]

@bmajz what do you think about contributing these typings to the @types npm organization? the guide to doing this is here: http://definitelytyped.org/guides/contributing.html. it seems like the scoped packages issue has been resolved, and this would have to be published as @types/slack__client.

to be clear, we still plan to author in TypeScript in a future major version. i see publishing to @types as a stop-gap solution. since installing that package is strictly opt-in, i don't think my previous reason for not merging (no types is better than wrong types) applies. if the types are wrong, the community can either self-correct by working together in the DefinitelyTyped repo, or you can uninstall the package and fallback to dealing with anys again.

[bmajz]

I'll look into it ASAP

…

Sent from my phone

On Oct 3, 2017, at 3:52 PM, Ankur Oberoi ***@***.***> wrote: @bmajz what do you think about contributing these typings to the @types npm organization? the guide to doing this is here: http://definitelytyped.org/guides/contributing.html. it seems like the scoped packages issue has been resolved, and this would have to be published as @types/slack__client. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[bilby91]

Awesome! We have a few enhancements to the previously posted typing that we want to contribute back.

[aoberoi]

not merging for reasons in conversation

[slawiko]

@bmajz how it goes? Did you add these types in DefinitelyTyped? Could I help you with it?

[orta]

There is already the start of a type definition for this SDK ( DefinitelyTyped/DefinitelyTyped#12195 ) in definitely typed, it'd need updating to use the new @slack/client module name, but updating that is probably the right way to go at this point

[aoberoi]

hey everyone! take a look at v4.0.0, which is written using TypeScript! i hope you like it.

i'll close this issue now, but if you're looking for type definitions for v3.x, this is still the right place to find them.

[orta]

🎉