[TS 5.4.0-beta] .d.ts emit adding an inline type import instead of utilising existing import

[acutmore]

🔎 Search Terms

declaration emit inline type import resolution

🕗 Version & Regression Information

• This changed between versions 5.3.3 and 5.4.0-beta
• This changed in PR #56857

⏯ Playground Link

https://www.typescriptlang.org/dev/bug-workbench/?emitDeclarationOnly=true&baseUrl=true&ts=5.4.0-beta#code/PTAEAEBMFMGMBsCGAnRAXAlgewHYCgQIBnACywHcBRAWwzQLHFIprrWkgDEN5oAuUIgAOQ4DkQA3DAHN0WZMHIZI06GgC0AM0Sw08gJ4A6SIbRE8DCJp7Rx1fqHgYARsAw4YAD2OnzGakLyaKAAVIJEoAAi0NoArvBolJ6ByMGayFjUoABEhm447Mji8MAwcQkA+tDJQUTZANx41SnBAN5RMYjxiTWp4aBlXQmgAL6NzUGh-QBCiETQAOrukBSg6Zk5ee6FxYrLFETAznPQSh4UPmYNFoTg1rx2Dk6u29BFiCWD3VW9ZpfmEz67S+wzmoFm8yW53IozWGSyuT20MOx3mZxW5GuljuNkeAme+R2HyRGJRJ3RFxMZjwr2Q2lg0HBJyhGNArRGTV+oFp9MZEMW+3IAGFcEQ0MhYrp5GyOTAEChGbBRcF+SyKAJVYKRTgxRKpchxlyQSrmYLGjdGPdbIh7PiXOpYph4IdSArIOo0PohNBDkoVGp1EqcNZpP8aQFJp7vaAAKoASThG1yeUMeQJy2q10BwR5OkZS39aE4Oj0yH0bLwoCr3J1aEQBQwcmQAAohAqCmryAJ44ZNdCAJQCCRYZSNDnYq14wQiMSSGRNvaFrQlgy+cMtNkF1RFldlkaJhGp4BH54Op0ukhuj1en2L7eB3AhrFypDIRXKmti+uYJsCLdqYt9X0ABtABydwvwbJtQIAXXNQgAFZDAAZgECoKiIfQChINQMFgH4WgqZ4KgARl7U1oWMTpuksJCABYBH8Fpm2TY80xcQk3l2Y0CNqbJ+3IyFBU5DdjVAAB5ZwACs4DQQx0mgaAAC9oGbVpK2rCC6yg0s8BGftzSYyMbymMF0Mw7DcPw7MiJcUiD02FN2JeDxM0aIA

💻 Code

// @declaration
// @showEmit
// @showEmittedFile: app/navigator/widget-factory.d.ts

// @filename: lib/index.d.ts
import * as DefaultExport from "./internal/default_exports";
export { DefaultExport as default };
export * as BaseWindow from "./internal/windows/basewindow.d.ts";

// @filename: lib/internal/default_exports.d.ts
export { default as BaseWindow } from "./windows/basewindow";

// @filename: lib/internal/windows/basewindow.d.ts
interface BaseWindow {}
export interface BaseWindowConstructor {}
declare const BaseWindow: BaseWindowConstructor;
export default BaseWindow;


// @filename: lib-utils/shared-types/widget-config.d.ts
import type UI from "../../lib/index";
export interface WidgetFactory {
    instantiator(parentWindow: UI.BaseWindow): void;
}

// @filename: app/navigator/widget-factory.ts
import {WidgetFactory} from "../../lib-utils/shared-types/widget-config";

declare const instantiator: WidgetFactory['instantiator'];

// 5.3: __synthetic_export_lib_1.BaseWindow.default
// 5.4: import("../../lib/internal/default_exports").BaseWindow
export default Object.freeze({
    instantiator
});

// Our tools inject this before we pass the file to TS.
import type * as __synthetic_export_lib_1 from "../../lib/index";

🙁 Actual behavior

// app/navigator/widget-factory.d.ts
declare const _default: Readonly<{
    instantiator: (parentWindow: import("../../lib/internal/default_exports").BaseWindow) => void;
}>;
export default _default;
import type * as __synthetic_export_lib_1 from "../../lib/index";

🙂 Expected behavior

// app/navigator/widget-factory.d.ts
declare const _default: Readonly<{
    instantiator: (parentWindow: __synthetic_export_lib_1.BaseWindow.default) => void;
}>;
export default _default;
import type * as __synthetic_export_lib_1 from "../../lib/index";

Additional information about the issue

More background in previous conversation around this: #38111 and this public blog post: https://www.bloomberg.com/company/stories/10-insights-adopting-typescript-at-scale/

The JavaScript runtime we are using TypeScript on is not based on npm style packages (no node_modules resolution or package.json manifest). For the .d.ts that we publish to be able to work consistently across different versions it's important that the inter-package import paths only resolve to the 'public' files and not internal implementation .d.ts files.

To encourage TypeScript to pick these safer paths when emitting .d.ts we add unused imports to the bottom of each file before passing them to the TS compiler.

In general this is still working in 5.4.0-beta, however during our testing at least 8 packages's .d.ts emit regressed with the beta. The playground above re-creates one of these.

[acutmore]

cc: @andrewbranch - @robpalme suggested you might be interested in this

[Andarist]

Since this was broken by my PR, I will investigate how hard it would be to reuse the existing imports. There is actually some logic that should prefer them already but perhaps it's applied at a different stage of the whole selection process.

[acutmore]

thanks @Andarist !

[fatcerberus]

@Andarist You're like the walking embodiment of "move fast and break things" 🚎

[Andarist]

I can only be as good as the existing test cases :P

[Andarist]

So this one is already fixed by one of my existing PRs: https://github.com/microsoft/TypeScript/pull/56100 (I just pushed out this test case there, could you also verify that the output looks like you expect it to?). Unfortunately, I have some PR feedback to address there 😅 I'll prioritize looking into this in the coming week

[RyanCavanaugh]

@weswigham probably worth taking another look at the linked PR and seeing what we might do here

[acutmore]

So this one is already fixed by one of my existing PRs: https://github.com/microsoft/TypeScript/pull/56100 (I just pushed out this test case there, could you also verify that the output looks like you expect it to?)

Cloning the https://github.com/microsoft/TypeScript/pull/56100 branch and linking that into our tooling, I can confirm that this issue is fixed; the .d.ts emit is as expected, using the existing module level type imports to access the type.