Repository navigation
tsc --build mode: generated .d.ts files causing 'Duplicate identifier' errors in downstream packages #25338
Description
TypeScript Version: 3.0.0-dev.20180630
Search Terms:
build mode, duplicate identifier
Steps to Reproduce
Set up Ryan Cavanaugh (@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).
Activity
RyanCavanaugh commentedon Jul 2, 2018
Wesley Wigham (@weswigham) Thoughts on this? Can we emit a
/// <reference types="node" />directive instead, or should we be intelligently de-duping these?ref. #25278
mhegazy commentedon Jul 2, 2018
Looks like a duplicate/related to:
#24599, #25358, #25345, #25186
weswigham commentedon Jul 2, 2018
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 forimporttypes, AFAIK. In this case, the ref is emitted as a requirement because"url"resolved to an ambient module -trackReferencedAmbientModuleas is just adds a triple-slash ref.Do we have a good algorithm on when it should be a deep import vs a
reference typesstyle import? Just whenever we can get a non-relative module specifier for the file (is<reference types="library/deep/file" />valid?)?weswigham commentedon Jul 2, 2018
Ryan Cavanaugh (@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 commentedon Jul 3, 2018
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
typesand apathreference to (potentially) the same thing!