TypeScript icon indicating copy to clipboard operation
TypeScript copied to clipboard

Module/interface augmentation doesn't preserve import for declaration emit

Open OliverJAsh opened this issue 2 years ago β€’ 8 comments

πŸ”Ž Search Terms

  • project references
  • module/interface augmentation

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about project references.

⏯ Playground Link

No response

πŸ’» Code

Full reduced test case (contents inlined below): https://github.com/OliverJAsh/ts-project-references-augmentation-bug

package.json:

{
  "dependencies": {
    "fp-ts": "^2.16.1",
    "typescript": "^5.2.2"
  }
}

app/tsconfig.json:

{
    "compilerOptions": {
        "rootDir": "../",
        "outDir": "../target",
        "composite": true
    }
}

app/index.ts:

import * as ReadonlyTuple from 'fp-ts/ReadonlyTuple';

export const Apply = ReadonlyTuple.getApply({ concat: a => a });

demos/tsconfig.json:

{
    "compilerOptions": {
        "rootDir": "../",
        "outDir": "../target"
    },
    "references": [
        { "path": "../app/tsconfig.json" },
    ]
}

demos/index.ts:

import { Apply } from "../app/index";

Apply;

πŸ™ Actual behavior

A module in the "app" project imports the file node_modules/fp-ts/lib/ReadonlyTuple.d.ts. This file uses module and interface augmentation. Excerpt:

export declare const URI = 'ReadonlyTuple'
/**
 * @category type lambdas
 * @since 2.5.0
 */
export type URI = typeof URI
declare module './HKT' {
  interface URItoKind2<E, A> {
    readonly [URI]: readonly [A, E]
  }
}

When I later try to reference the "app" project from another project ("demos") using project references, I get a type error because the module/interface augmentation is not being applied:

$ yarn run tsc --build --verbose demos/tsconfig.json
yarn run v1.22.19
warning package.json: No license field
$ /Users/oliver/Code/reduced-test-cases/ts-project-references-augmentation-bug/node_modules/.bin/tsc --build --verbose demos/tsconfig.json
[13:49:06] Projects in this build:
    * app/tsconfig.json
    * demos/tsconfig.json

[13:49:06] Project 'app/tsconfig.json' is out of date because output file 'target/app/tsconfig.tsbuildinfo' does not exist

[13:49:06] Building project '/Users/oliver/Code/reduced-test-cases/ts-project-references-augmentation-bug/app/tsconfig.json'...

[13:49:06] Project 'demos/tsconfig.json' is out of date because output file 'target/demos/index.js' does not exist

[13:49:06] Building project '/Users/oliver/Code/reduced-test-cases/ts-project-references-augmentation-bug/demos/tsconfig.json'...

target/app/index.d.ts:1:63 - error TS2344: Type '"ReadonlyTuple"' does not satisfy the constraint 'keyof URItoKind2<any, any>'.

1 export declare const Apply: import("fp-ts/lib/Apply").Apply2C<"ReadonlyTuple", unknown>;
                                                                ~~~~~~~~~~~~~~~


Found 1 error.

πŸ™‚ Expected behavior

No error. The module/interface augmentation should be applied in the dependant project when it consumes the type declarations of the referenced project.

Additional information about the issue

I am able to workaround the issue by adding an explicit import of fp-ts/ReadonlyTuple in demos/index.ts.

OliverJAsh avatar Nov 24 '23 14:11 OliverJAsh

The issue here is not project references but how d.ts is emitted. Assigning to @weswigham who knows more about this area to see if this is working as intended or there should be error or import should be preserved and can it even be determined.

For repro purposes you just need to build "app" and you will see that:

// app/index.ts
import * as ReadonlyTuple from 'fp-ts/ReadonlyTuple';
export const Apply = ReadonlyTuple.getApply({ concat: a => a });

generates following d.ts

// app/index.d.ts
export declare const Apply: import("fp-ts/lib/Apply").Apply2C<"ReadonlyTuple", unknown>;

Error goes away if the import is preserved.

sheetalkamat avatar May 17 '24 18:05 sheetalkamat

We used to have machinery to preserve required augmentations by adding /// <references to the declaration file (though determining this one is required would certainly be weird, with how indirect it is) - but we gave that up, reasoning that consumers should have to include those augmentations manually.

I'm not sure we have a viable remediation since we did https://github.com/microsoft/TypeScript/pull/57681. We could add imports, but isn't that basically the same as adding /// references?

weswigham avatar May 17 '24 19:05 weswigham

@jakebailey thoughts?

weswigham avatar May 17 '24 19:05 weswigham

I see .. I forgot about that PR .. but even the. I thought this would preserve the import as needed.. instead of generating .. but given how this type materializes may be not ..

sheetalkamat avatar May 17 '24 20:05 sheetalkamat

I also think among users write these imports is better than we generating something but not biased towards either approach

sheetalkamat avatar May 17 '24 20:05 sheetalkamat

I guess we didn't think about import elision, yeah. Under ID, the imports are never erased, but obviously there's no way to force it to stay otherwise besides verbatimModuleSyntax, writing import 'fp-ts/ReadonlyTuple' (maybe), or writing a plain reference.

I'm not sure how we can do this properly; the whole point of #57681 was to explicitly not rely on whole program info for preserving imports. That implies that manual intervention could be needed, but TS is clearly removing an import too.

jakebailey avatar May 17 '24 23:05 jakebailey

What I don’t understand is why there was cannot write type or unaccessible type error at least .. is it because augmentation is not considered for writing type ..

sheetalkamat avatar May 18 '24 01:05 sheetalkamat

The augmentation is what makes the type instantiation valid to make - otherwise the type is unconstructable. (Basically, the Apply type doesn't know how to make a ReadonlyTuple out of its' arguments without the augmentation included.)

Still dunno what we should do here. Declaration emit is defined as eliding any imports not explicitly referenced by an annotation, and we no longer have logic to add references to augmentations we think you need to make the types in the declaration file work. Is our stance "oh, whoever uses this declaration file should include the augmentation themselves"?

weswigham avatar May 20 '24 20:05 weswigham

I'm encountering this issue more frequently after upgrading to TypeScript 5.5. Were there any changes related to this?

OliverJAsh avatar Jun 20 '24 20:06 OliverJAsh

The aforementioned #57681 means that no references are generated, nor preserved unless explicitly annotated to do so. I guess if you didn't test the beta/rc, you might see it as a recent regression, but nothing has been changed on this front since that PR. Of course your issue was filed before that PR, so the old behavior was already buggy (since we couldn't accurately generate references anyway).

A workaround is to explicitly write this somewhere:

/// <reference types="fp-ts/ReadonlyTuple" preserve="true" />

jakebailey avatar Jun 20 '24 21:06 jakebailey

@jakebailey Prior to 5.5, Typescript would include the triple slash reference path to a local d.ts, if it was determined that I wrote my own global typings file to augment a module:

// module.d.ts
declare module 'eslint-plugin-react/configs/jsx-runtime.js' {
  import type { TSESLint } from '@typescript-eslint/utils';
  const _default: TSESLint.FlatConfig.Config;
  export default _default;
}
// eslint.config.ts
export { default as jsxRuntime } from 'eslint-plugin-react/configs/jsx-runtime.js';

To be exported as dist/esliint.config.js and the d.ts output, exported as dist/eslint.config.d.ts:

/// <reference path="../modules.d.ts" />
export { default as jsxRuntime } from 'eslint-plugin-react/configs/jsx-runtime.js';
declare const _default: import("@typescript-eslint/utils/ts-eslint").FlatConfig.ConfigArray;
export default _default;

However, this case breaks after your PR was merged.

psychobolt avatar Jul 05 '24 23:07 psychobolt

There's just no way to determine intent here without getting it wrong the opposite way around. You could have refactored your code the wrong way and that reference may not have been emitted.

You can explicitly write a reference with preserve=true if you intend for that to be included in the program.

jakebailey avatar Jul 05 '24 23:07 jakebailey

On my observation, with preserve=true, the reference is not preserved in d.ts files. Only in the emitted .js files. However, Ts-Eslint will parse the d.ts files first before resolving the .js files. Would it be ideal to also keep the reference in d.ts emitted declaration files?

psychobolt avatar Jul 06 '24 00:07 psychobolt

I mean to write:

/// <reference path="..." preserve="true" />

https://devblogs.microsoft.com/typescript/announcing-typescript-5-5/#simplified-reference-directive-declaration-emit

This is guaranteed to be emitted in declaration files. If not, please do file a separate bug.

jakebailey avatar Jul 06 '24 02:07 jakebailey

Discovered it was a mistake on my end. I had the reference after my import. It is working for me after hoisting to the top.

psychobolt avatar Jul 06 '24 04:07 psychobolt

Ah, yeah, they have to go at the very top, and we don't exactly warn about that...

jakebailey avatar Jul 06 '24 13:07 jakebailey