Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
OliverJAsh opened this issue Nov 24, 2023 · 10 comments
Open
Assignees
Labels
Domain: Declaration Emit The issue relates to the emission of d.ts files Needs Investigation This issue needs a team member to investigate its status.

Comments

@OliverJAsh
Copy link
Contributor

πŸ”Ž 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.

@andrewbranch andrewbranch added the Needs Investigation This issue needs a team member to investigate its status. label Nov 27, 2023
@andrewbranch andrewbranch added this to the TypeScript 5.4.0 milestone Nov 27, 2023
@sheetalkamat
Copy link
Member

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 sheetalkamat changed the title Module/interface augmentation in referenced project is not applied to dependant project Module/interface augmentation doesnt preserve import for declaration emit May 17, 2024
@sheetalkamat sheetalkamat added the Domain: Declaration Emit The issue relates to the emission of d.ts files label May 17, 2024
@OliverJAsh OliverJAsh changed the title Module/interface augmentation doesnt preserve import for declaration emit Module/interface augmentation doesn't preserve import for declaration emit May 17, 2024
@weswigham
Copy link
Member

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 #57681. We could add imports, but isn't that basically the same as adding /// references?

@weswigham
Copy link
Member

@jakebailey thoughts?

@sheetalkamat
Copy link
Member

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
Copy link
Member

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

@jakebailey
Copy link
Member

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.

@sheetalkamat
Copy link
Member

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 ..

@weswigham
Copy link
Member

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"?

@OliverJAsh
Copy link
Contributor Author

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

@jakebailey
Copy link
Member

jakebailey commented Jun 20, 2024

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" />

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Declaration Emit The issue relates to the emission of d.ts files Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

No branches or pull requests

5 participants