Node-slack-sdk: Move @types/node from devDependencies to dependencies

[swftvsn]

Description

My typescript project does not compile after upgrading to 4.0.1.

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

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

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

• [ x] I've read and understood the Contributing guidelines and have done my best effort to follow them.
• [ x] I've read and agree to the Code of Conduct.
• [ x] 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.0.1

node version: v8.9.1

OS version(s): Mac High Sierra

Steps to reproduce:

1. Include @slack/client to project using npm install @slack/client
2. Try to compile with tsc

Expected result:

The project compiles

Actual result:

node_modules/@slack/client/dist/util.d.ts(29,47): error TS2339: Property 'callbackify' does not exist on type 'typeof "util"'.

Looking at the generated util.d.ts file, it contains
export declare const callbackify: typeof util.callbackify;

This should probably be more complex type?

I'm trying to compile using typescript 2.7.2 by running node_modules/typescript/bin/tsc

[assafshp]

I'm also getting this error

[aoberoi]

Thanks for reporting! Can you share the contents of your tsconfig.json file?

[swftvsn]

This is what I have:

{
  "compilerOptions": {
    "lib": ["es6", "es2015.promise", "dom"],
    "module": "commonjs",
    "strict": true,
    "noImplicitAny": true,
    "strictNullChecks": false,
    "outDir": "build",
    "sourceMap": false,
    "target": "es6",
    "typeRoots" : ["./src/_shared/types", "./node_modules/@types"]
  },
  "include": [
    "src/**/*.ts",
    "spec/**/*.ts"
  ]
}

[clavin]

I was able to reproduce this issue, except I could only do so using an old version of @types/node--obviously, any version in which util.callbackify doesn't exist.

I figured out that the version of @types/node that util.callbackify was added was 8.0.33. Thus, the compatible range of versions of @types/node is >=8.0.33.

In this specific case, I feel that the solution is probably to just run npm install --save-dev @types/node@^8.0.0 (modifying for your specific environment, i.e. yarn or --save if needed).

---

While testing, I also found that our target version of @types/got is incompatible with @types/node@<=8.0.15:

node_modules/@types/got/index.d.ts(34,19): error TS2694: Namespace '"http"' has no exported member 'IncomingHttpHeaders'.

[swftvsn]

Thanks @clavin and @aoberoi ! I can confirm that adding the node types as dev dependency gets rid of this error.

It is somewhat counter intuitive though, as if I understand correctly node version 6 and above is supported, but you must have types from 8 or later? This also makes developing against 6 or 7 more error prone, as you might use stuff that is not available in reality.

This I think is a problem, as we run our code in Firebase Cloud Functions (Google Cloud Functions) and they are still on version 6. I just checked AWS Labmda too, and they support either 4 or 6.

Would it be possible to express the typing some other way to properly support 6 and 7?

[clavin]

I can definitely agree with the statement about the required @types/node: it’s confusing to have these mismatched requirement versions.

In theory, as long as you compile your TypeScript sources ahead of time and run the generated JS output (which, on GCF and Lamda you should do as compilation on the server would be a waste of resources) then you won’t run into this type error, and your code should run fine (again, as you said, watch out for unimplemented APIs). Versions of node that don’t have util.callbackify are [safely] polyfilled as needed for this package, so 6 and 7 are good to go runtime-wise.

Alas, I have no other solution for you except to use these newer node typings in the meantime while this issue is sorted out.

---

As for resolving this issue with the package, it’s easy enough to fix the original problem by giving the callbackify export in util.ts an explicit type so the generated type definitions (.d.ts files) don’t try to use a reference to the @types/node package:

interface CallbackifyType {
  // ...
}

export const callbackify: CallbackifyType = // ...

This, however, doesn’t resolve the issue I was getting with using the @types/got package with older node typings—a problem that I can’t think of a great solution for.

[aoberoi]

One solution I can think of would be to move the @types/node dependency from devDependencies to dependencies. However, this worries me because it would be technically incorrect when running on older node versions. In practice, as long as our tests passed on all supported versions of node, any issues that stem from that should be caught before a bad release is made. It would be up to the code maintainers to make sure all the older versions of node are considered.

Right now, I prefer this solution over copying the type from the @types/node package into this package, because keeping definitions in sync is a worse problem.

[aoberoi]

There is some guidance regarding when to put a @types package in dependencies versus devDependencies in the following issue: https://github.com/Microsoft/types-publisher/issues/81

The advice I see there is to put @types/node in dependencies, but to use a specifier like >= 6. I don't think this would help in our case, because the consumer of the package would presumably install @types/[email protected], in which util.callbackify still will not have a type.

I think its worth implementing the solution in my previous comment because at worst, the consumer of the package will end up with 2 versions of @types/node in their node_modules tree, but the right version will be exposed to the right packages. As long as at runtime there is an implementation that works on lower versions of node (checked by CI) then we should be safe.