nodeBuiltins depend on @types/node

[jablko]

Like non-built-in modules, add a dependency (on @types/node in this case) given the following (unless there's already a dependency, on @types/node or on the specified module):

import * as http from "http";

Otherwise it errors:

error TS2307: Cannot find module 'http' or its corresponding type declarations.

[sandersn]

It's been a while since I looked at this code. Can you give a larger example of a project whose publish is wrong before this change and would be fixed by it?

[jablko]

I think @types/standard-engine and @types/undertaker are current examples. This change adds the missing dependency on @types/node to both:

@types/standard-engine

$ npm install @types/standard-engine
$ > index.ts
$ tsc index.ts
node_modules/@types/standard-engine/index.d.ts:7:22 - error TS2307: Cannot find module 'os'.

7 import { type } from 'os';
                       ~~~~

❌

$ npm install @types/node
$ tsc index.ts

✔️

@types/undertaker

$ npm install @types/undertaker
$ > index.ts
$ tsc index.ts
node_modules/@types/undertaker/index.d.ts:9:24 - error TS2307: Cannot find module 'stream'.

9 import { Duplex } from "stream";
                         ~~~~~~~~

node_modules/@types/undertaker/index.d.ts:10:30 - error TS2307: Cannot find module 'events'.

10 import { EventEmitter } from "events";
                                ~~~~~~~~

node_modules/@types/undertaker/index.d.ts:25:56 - error TS2503: Cannot find namespace 'NodeJS'.

25         (done: (error?: any) => void): void | Duplex | NodeJS.Process | Promise<never> | any;
                                                          ~~~~~~

❌

$ npm install @types/node
$ tsc index.ts

✔️

[jablko]

On second thought, while a dependency on @types/node (or @types/ the specified module) is necessary, it's not sufficient:

$ npm install @types/standard-engine
$ > index.ts
$ tsc --types standard-engine index.ts
node_modules/@types/standard-engine/index.d.ts:7:22 - error TS2307: Cannot find module 'os'.

7 import { type } from 'os';
                       ~~~~

$ npm install @types/node
$ tsc --types standard-engine index.ts
node_modules/@types/standard-engine/index.d.ts:7:22 - error TS2307: Cannot find module 'os'.

7 import { type } from 'os';
                       ~~~~

The types must also /// <reference types="node" />.

I've updated this change to throw an error where before it just added the missing dependency. Linting all files in a package, not just the tests, would also catch these deficiancies?

[peterblazejewicz]

fyi: fixing import { type } from 'os'; in DefinitelyTyped/DefinitelyTyped#44213 as being result of simple mistype,
kudos to @jablko though for his work!

[jablko]

I opened microsoft/dtslint#286, which is an attempt to address this PR in a more general way: Instead of catching libraries that import a nodeBuiltins module without a /// <reference types="node" /> (or a dependency on @types/ the specified module), that PR attempts to catch libraries that error on import.

[jablko]

I've updated this PR again, this time to add dependencies on @types/${dependency} (just like all other dependencies) instead of @types/node. The @types/${built-in module} packages are introduced in DefinitelyTyped/DefinitelyTyped#44429

[sandersn]

The present state of this is an interesting idea, but it depends on DefinitelyTyped/DefinitelyTyped#44429 to split up @types/node into @types/buffer (for example).

Since this PR isn't ready to go yet, and has changed substantially since its creation anyway, can you re-create it at https://github.com/microsoft/DefinitelyTyped-tools? It's the new monorepo that contains all of DT's publishing infrastructure.

I'm going to look at your other PRs and merge+port those that are simple enough and ready to go. I'll comment on any others that I'd like you to recreate the PR at DefinitelyTyped-tools.