tsc --build mode: generated .d.ts files causing 'Duplicate identifier' errors in downstream packages

[yortus]

TypeScript Version: 3.0.0-dev.20180630

Search Terms:
build mode, duplicate identifier

Steps to Reproduce

Set up @RyanCavanaugh's learn-a sample repo as per his instructions. Now run lerna add @types/node to add Node.js typings to all three packages. Run tsc -b packages --force to confirm it still builds fine. Now add the following code to pkg1/src/index.ts:

// CASE1 - no build errors in pkg1, but 'duplicate identifier' build errors in pkg2
// import {parse} from 'url';
// export const bar = () => parse('bar');

// CASE2 - no build errors in pkg1 or in downstream packages
// import {parse, UrlWithStringQuery} from 'url';
// export const bar = (): UrlWithStringQuery => parse('bar');

// CASE3 - no build errors in pkg1 or in downstream packages
// export declare const bar: () => import("url").UrlWithStringQuery;

// CASE4 - no build errors in pkg1, but 'duplicate identifier' build errors in pkg2
// import {parse} from 'url';
// type UrlWithStringQuery = import("url").UrlWithStringQuery;
// export const bar = (): UrlWithStringQuery => parse('bar');

Uncomment one case at a time and run tsc -b packages --force.

Actual behaviour / Expected behavior:

Cases 1 and 4 cause a deluge of build errors in pkg2. But with cases 2 and 3, there are no build errors. The important difference with cases 1 and 4 seems to be the first line in the generated pkg1/lib/index.d.ts:

/// <reference path="../node_modules/@types/node/index.d.ts" />

Cases 2 and 3 don't generate this line. When pkg2 is built in case 1 and 4, it includes two identical copies of @types/node declarations at different paths, and that causes the 'duplicate identifier' errors.

Perhaps this is all expected behaviour, however it seems pretty confusing. There are no build errors or warnings in pkg1 for any of these 4 cases, but the downstream build behaviour is very sensitive to the exact style of the exported declarations.

I think emitting /// <reference path... comments in build mode needs further consideration as to how not to clash with downstream declarations. Also should all four cases result in the same build behaviour? It's not obvious (to me at least) why they produce different emits, where some work and some fail downstream.

Related Issues:

Originally reported in #3469 (comment).

[RyanCavanaugh]

@weswigham Thoughts on this? Can we emit a /// <reference types="node" /> directive instead, or should we be intelligently de-duping these?

ref. #25278

[mhegazy]

Looks like a duplicate/related to:
#24599, #25358, #25345, #25186

[weswigham]

Can we emit a /// <reference types="node" /> directive instead, or should we be intelligently de-duping these?

We actually track what references directives were used in printing the file and preserve/create them. I think the issue in this case is that there's no directive to preserve because it was implicitly "included" in the build via the context, so when the declaration emitter sees that it is a required dependency, it writes it out as a triple-slash reference. Today, the only way you get a /// <reference types="node" /> directive out of declaration emit is by inputting one (as the mapping of name -> path is only created when one exists). We only do the reverse-module-resolution type stuff for import types, AFAIK. In this case, the ref is emitted as a requirement because "url" resolved to an ambient module - trackReferencedAmbientModule as is just adds a triple-slash ref.

Do we have a good algorithm on when it should be a deep import vs a reference types style import? Just whenever we can get a non-relative module specifier for the file (is <reference types="library/deep/file" /> valid?)?

[weswigham]

@RyanCavanaugh #25278 is unrelated, I'm pretty sure. In #25278 the types ref actually needs to be present (as a local path) - the issue is that we produce syntactically invalid output.

[weswigham]

Haaaaaaaaaaaaaaaaaaaaaaah, so this was even simpler to fix, I think. We actually do already pass thru the mapping already - we just weren't checking if the triple-slash ref we wanted to add to ensure a file was referenced was also a triple-slash types ref; this wasn't clear at first since I wasn't aware that the issue was they it was emitting both a types and a path reference to (potentially) the same thing!