@slack/client incompatible with @types/[email protected]

[igrayson]

Description

If a typescript project is using @types/[email protected], it cannot use @slack/client.

What type of issue is this? (place an x in one of the [ ])

• bug
• enhancement (feature request)
• question
• documentation related
• testing related
• discussion

Requirements (place an x in each of the [ ])

• 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 searched for any related issues and avoided creating a duplicate issue.

---

Bug Report

Filling out the following details about bugs will help us solve your issue sooner.

Reproducible in:

@slack/client version: 4.3.1

node version: 8.9.1

OS version(s):

Steps to reproduce:

$ mkdir demo && cd demo
$ npm install [email protected] @types/[email protected] @slack/[email protected]
$ touch index.ts
$ tsc --lib es2017 index.ts
node_modules/@types/p-cancelable/index.d.ts:10:11 - error TS2430: Interface 'PCancelableConstructor' incorrectly extends interface 'PromiseConstructor'.
  Types of property 'prototype' are incompatible.
    Type 'PCancelable<any>' is not assignable to type 'Promise<any>'.
      Types of property 'cancel' are incompatible.
        Type '() => void' is not assignable to type '<U>(reason?: any) => Promise<U>'.
          Type 'void' is not assignable to type 'Promise<U>'.

10 interface PCancelableConstructor extends PromiseConstructor {
             ~~~~~~~~~~~~~~~~~~~~~~

node_modules/@types/p-cancelable/index.d.ts:18:15 - error TS2430: Interface 'PCancelable<T>' incorrectly extends interface 'Promise<T>'.
  Types of property 'cancel' are incompatible.
    Type '() => void' is not assignable to type '<U>(reason?: any) => Promise<U>'.
      Type 'void' is not assignable to type 'Promise<U>'.

18     interface PCancelable<T> extends Promise<T> {
                 ~~~~~~~~~~~

Expected result:

compiles

Actual result:

doesn't compile

[igrayson]

Is there a reason that @slack/client declares @types/p-cancelable under dependencies?

[aoberoi]

@igrayson p-cancelable is used inside the RTMClient.

thanks for reporting, i'll try digging into what exactly should be done to resolve this, but i'm open to ideas.

[igrayson]

In our vended packages, we only put @types/* in our package.json under dependencies if they're being re-exported by that package.

The rest are declared as devDependencies, as they're only needed to compile the package, not consume it.

For @slack/client, I would try moving all of your @types/* to devDependencies (except for @types/retry; re-exported by your retry-policies interface).

We've actually learned to entirely avoid re-exporting types (can get you in different kind of trouble), but that's not an immediate concern here.

[aoberoi]

yes, this is something i've heard before, but as far as i can tell the jury is still out on whether this is truly a best practice.

the question of dependencies versus devDependencies has been discussed in previous issues such as #514 and #582 (although these were about @types/node, it was a similar problem). the guidance from the TypeScript team themselves seems to be for package authors to include all the @types/* packages in the dependencies section of package.json. See: microsoft/types-publisher#81. currently, we're following this guidance and haven't run into something that couldn't be fixed without bending that.

in this case, i believe the typings for bluebird are not correct. in its declarations, they are merging their own method definitions into the native JavaScript Promise<T> interface, instead of defining their own type that extends Promise<T> (see merging interfaces). this creates a requirement on all other types that extend Promise<T>, such as PCancellable<T>, to be only provide compatible API. the error message you've shared shows that PCancellable's and bluebird's definitions of the cancel() method are incompatible because the return type void is not assignable to some Promise<U>.

i think the best way to resolve this issue would be to improve the type declarations for bluebird. from reading through the source, it seems like the author(s) of that declaration file know they've done some things wrong, and have even marked this particular cancel() method with a cryptic TODO. https://github.com/DefinitelyTyped/DefinitelyTyped/blob/7c7d2678b43959f5b59dd7af9974c9ff911de99c/types/bluebird/v2/index.d.ts#L478

[freezy]

I got a similar problem in my CI with latest v4.4.0.

$ mkdir demo
$ cd demo
$ npm install [email protected] @slack/[email protected]
$ echo import { RTMClient } from '@slack/client'; > index.ts
$ tsc --lib es2017 index.ts
node_modules/@slack/client/dist/util.d.ts:32:56 - error TS2304: Cannot find name 'AsyncIterable'.

32 export declare function awaitAndReduce<T, U>(iterable: AsyncIterable<T>, callbackfn: (previousValue: U, currentValue: T) => U, initialValue: U): Promise<U>;
                                                          ~~~~~~~~~~~~~

Posting it here because it's a Typescript related problem, let me know if you prefer me creating a new issue.

[aoberoi]

@freezy i'm fairly certain that the solution to your problem would be to include the esnext.asynciterable value in the lib option.

this is a new requirement since the v4.4.0 release, so thanks for catching it! i'm not sure how to document this appropriately, but i'll create a new issue to capture the need.

[freezy]

How about just adding

/// <reference lib="esnext.asynciterable" />

to your util.d.ts? I don't think the dependents of your lib should maintain your dependencies...

[aoberoi]

@freezy i agree, and making dependents maintain the deps was not the intention.

[igrayson]

@aoberoi

Bluebird typings may be improve-able (although there could be a reason they took the approach they did; they more scrutiny than most libraries), but that is perhaps orthogonal to this issue.

I think @slack/client should be as un-opinionated as possible about the technique used to declare typings of libraries that are not themselves relevant to any interface exported by @slack/client.

Re: @types/node references -- there's a fundamental difference between those issues and this one: p-retry does not occur in @slack/client's exported interface; it is wholly an internal, implementation detail of the library, and consumers of your interface probably should be as ignorant as possible that you use it. (I express this as an assertion, but I'm curious if you agree).

@types/node on the other hand is directly re-exported by your interface, and consumers wouldn't be able to compile with @slack/client without also installing @types/node.

What do you think?

[aoberoi]

@igrayson i understand your point about internal versus exported or exposed types. i also think its valid.

but the downside of separating specific @types/* packages into those that are internal versus exported is the likely unnecessary maintenance burden. my point of view is based on following the guidelines set fourth by the typescript publishing guidelines. i think these guidelines represent, on balance, what's currently thought to be best for the entire ecosystem. in order to take the approach you're suggesting, every package publisher in the entire ecosystem should take on the burden of introspecting their package for internal versus exported types. to be honest, i think a future version of typescript might be able to solve this issue. the alternative seems both achievable and much more reasonable to me.

it's clear that we can debate this forever, so i think a reasonable approach would be to at least bring visibility to the problem with the bluebird types and inquire as to whether there is good reason for them to be doing what seems "bad" right now. its quite likely that a contribution to fixing those types (which the maintainers of this package may be able to help with) is much more valuable to the entire typescript community than for this package to make an exception to make it play better with bad actors.

[aoberoi]

i'm making my way through the DefinitelyTyped repo and looking at the bluebird type definitions. i noticed that in the latest major version (v3.x), the issue i cited is resolved. namely, in this version the Bluebird<R> (the main export) implements PromiseLike<R> instead of using declaration merging.

it seems like nobody has touched the 2.x type declarations in about a year.

if this is a blocker for you, it might be a potentially faster resolution to upgrade your usage to v3.x of bluebird.