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

@ts-ignore not included in .d.ts file when it's needed #38628

Closed
SephReed opened this issue May 18, 2020 · 24 comments
Closed

@ts-ignore not included in .d.ts file when it's needed #38628

SephReed opened this issue May 18, 2020 · 24 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@SephReed
Copy link

TypeScript Version: (version check command not supplied)

Search Terms:
@ts-ignore .d.ts

Code
For reasons within my codebase, the following code absolutely necessitates // @ts-ignore in order to disable an issue in which ProxiedAny<TYPE extends object> only takes objects for TYPE. Without this line, I get the error: Type 'TYPE' does not satisfy the constraint 'object'

export function isProxied<TYPE>(checkMe: TYPE | Sourcified<TYPE>)
  // @ts-ignore 
  : checkMe is ProxiedAny<TYPE>
{
  return checkMe && typeof checkMe === "object" && "_proxied" in checkMe;
}

Expected behavior:
When compiling, I would expect to find this //@ts-ignore included with the .d.ts file.

Actual behavior:
The .d.ts file is broken:
Screen Shot 2020-05-17 at 10 46 30 PM

Alternatives:
Maybe arg is TYPE should be able to do something like arg as unknown is TYPE in order to get around this issue. I'm checking if it's an object, and need a way to tell Typescript so.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jun 8, 2020
@RyanCavanaugh
Copy link
Member

ts-ignore does not imply anything other than hiding the error; any downstream effects of the error will still be present

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@SephReed
Copy link
Author

This is definitely broken. You just don't understand what I'm saying.

@RyanCavanaugh
Copy link
Member

@SephReed if you're not going to provide information to help me understand, what do you expect to happen?

@SephReed
Copy link
Author

Setting the tag "Working as Intended" before asking for clarity doesn't inspire much faith.

If you copy paste the code above and run tsc you will see the output .d.ts file is broken. I don't think tsc should generate broken .d.ts files. Obviously broken .js files from @ts-ignore would be user error, but .d.ts files should always be good.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 26, 2020

I don't think it changes the answer, but what I believe @SephReed is saying is that they believe that when compiling a .ts file with "declaration": true that any // @ts-ignore pragmas should be copied to the emitted .d.ts file, in order to preserve the intent of the original .ts code.

Of course they could simply write the code properly in the first place, instead of just ignoring errors:

export function isProxied<TYPE extends object>(checkMe: TYPE | Sourcified<TYPE>): checkMe is ProxiedAny<TYPE>
{
  return checkMe && typeof checkMe === "object" && "_proxied" in checkMe;
}

@RyanCavanaugh
Copy link
Member

I haven't heard anything that contradicted my initial understanding of the issue.

@ts-ignore silences errors at the use site and does nothing else; this is exactly its intended behavior.

It's not a magic spell that will cause all downstream effects of that error to go away. We don't know what your intent of ts-ignore was when you used it; maybe your .d.ts file targets some environment where that code will end up being valid. I would consider this particular use of ts-ignore to be incorrect.

These were exactly the kinds of things we cautioned would be limitations of ts-ignore when we added it and people told us it would be fine, no one would misapprehend what it was for, but here we are 🤷‍♂️

@SephReed
Copy link
Author

Of course they could simply write the code properly in the first place, instead of just ignoring errors:

I know you aren't working with my codebase, so you can't know this, but isProxied<TYPE extends object> does not fit the bill. It needs to be able to take any reference at all and tell if it is proxied.


If I import a .ts file that includes a @ts-ignore that relates to typing, then compile it with tsc, then import the exact same file again using a .js and .d.ts combo, it should work the same. Nothing about the typing should change between the .ts file and the .d.ts file.


I understand that I'm working on edge cases, and that it's really just a problem for me. But please don't say that .d.ts having different typings from the .ts files they were generated from makes any sense at all.

@SephReed
Copy link
Author

In the end, I have made very, very rare use of @ts-ignore, so it's super simple to add a post-build script to just reinject it into the typing files.

It's a problem solved for me. If you guys don't care that .d.ts files have different typings from .ts in this case, that's up to you.

mmkal added a commit to sequelize/umzug that referenced this issue Oct 31, 2020
ts-ignore comments are stripped from declaration files: microsoft/TypeScript#38628

It'd be possible to use some kind of codegen to add them back in, but it's easier and more correct to define what we need explicitly from the sequelize type definitions.
mmkal added a commit to sequelize/umzug that referenced this issue Oct 31, 2020
ts-ignore comments are stripped from declaration files: microsoft/TypeScript#38628

It'd be possible to use some kind of codegen to add them back in, but it's easier and more correct to define what we need explicitly from the sequelize type definitions.
mmkal added a commit to sequelize/umzug that referenced this issue Oct 31, 2020
ts-ignore comments are stripped from declaration files: microsoft/TypeScript#38628

It'd be possible to use some kind of codegen to add them back in, but it's easier and more correct to define what we need explicitly from the sequelize type definitions.
@mmkal
Copy link

mmkal commented Nov 25, 2020

@RyanCavanaugh this decision makes sense. I'm wondering though, what should library authors do that want to allow users of multiple typescript versions? Specifically, when there's a nice-to-have but non-essential feature. Example using the [...A, B] feature from typescript 4+ (source link)

// @ts-ignore
type Push_ts4<A extends unknown[], B> = [...A, B];
type VariadicTuplesPrefixesSupported = Push_ts4<[1, 2], 3> extends { length: 3 } ? "yes" : "no";
type Push<A extends unknown[], B> = VariadicTuplesPrefixesSupported extends "yes"
    ? Push_ts4<A, B>
    : Array<A[number] | B>;

The idea is that in recent typescript versions, Push<[1, 2], 3> is [1, 2, 3] whereas on old versions it'll be (1 | 2 | 3)[], which is less good, but okay and not inaccurate.

I tested this by installing a lower typescript version and compiling, which worked because the source code has the ts-ignore. But I forgot that it wouldn't end up in the declaration files that users actually use, and now I've got a bug report of the library causing compilation errors for old typescript versions: mmkal/handy-redis#250

What would you suggest I do here to un-break the downstream users, short of seding the d.ts file? I'm aiming to use the ts-ignore responsibly (albeit hackily), with the conditional type aiming to check what actually happened on the ignored line. Is there a better way, to load certain files based on typescript versions?

@MartinJohns
Copy link
Contributor

@mmkal downlevel-dts and typesVersions is as far as the TypeScript team is willing to go to support older versions in dts files.

See Ryans comment: #41685 (comment)

@DetachHead
Copy link
Contributor

this sounds absurd to me

@ts-ignore silences errors at the use site and does nothing else; this is exactly its intended behavior.

doesn't that make it completely useless then for packages intended to be imported downstream? i often need @ts-expect-error to suppress false positives in the compiler. that means anyone importing my package will get invalid d.ts files? how am i supposed to work around the issues i've found (ie. #46176, #46171, #43736) if my package is meant to be used downstream?

it'd be like if type casts were stripped. how is it at all intentional behavior to change the code at compiletime to emit invalid .d.ts files? is there something i'm missing here?

@andrewbranch
Copy link
Member

how am i supposed to work around the issues i've found

Can’t all those be worked around with an intersection?

@DetachHead
Copy link
Contributor

DetachHead commented Oct 12, 2021

@andrewbranch wow you're right, thanks!

however i still think it's an issue that the ignore comments behave this way. is there some reason behind it that i'm missing?

regardless, i think the dangers of //@ts-expect-error and especially //@ts-ignore are extremely underplayed in the documentation.

@RyanCavanaugh

These were exactly the kinds of things we cautioned would be limitations of ts-ignore when we added it and people told us it would be fine, no one would misapprehend what it was for, but here we are 🤷‍♂️

afaik nowhere in the documentation does it warn about this extremely dangerous behavior. especially for people publishing packages these errors will go undetected by any of their testing and will only be picked up by someone downstream trying to import them, so i'm not surprised people are misapprehending it

is it safe to say that //@ts-expect-error and //@ts-ignore should never be used in published packages that expose .d.ts files? is this documented anywhere?

@andrewbranch
Copy link
Member

I’m open to hearing feedback about how @ts-ignore should work, but I do think it’s useful to explore whether or not there are other viable workarounds. A conceptual issue I see with outputting @ts-ignore to .d.ts files is that it can be a bit opaque what type consumers will actually see. Often the compiler will produce an any in the face of an error so the bad type doesn’t propagate and cause errors in other places, but it’s really not obvious that you have an any and what other types that any will infect. On the other hand, if you hand-craft a workaround with type assertions, you can probably do better than any in many places, and where you do have an any, it will be obvious. For this reason, I think the strongest argument for @ts-ignore in .d.ts files would necessarily deal with common cases where other type-level workaround are not available. We would need to examine those cases and see if there’s a different solution that makes more sense first.

is it safe to say that //@ts-expect-error and //@ts-ignore should never be used in published packages that expose .d.ts files?

Not quite—it’s perfectly “safe” to use one of those in implementation code that doesn’t contribute to the type signature of any globals or module exports. It’s only bad news when the comment applies to something that will necessarily be copied to the declaration files, like a return type annotation of an exported function or something. But you may be right that the docs should do more to warn library authors about this.

@DetachHead
Copy link
Contributor

I do think it’s useful to explore whether or not there are other viable workarounds. A conceptual issue I see with outputting @ts-ignore to .d.ts files is that it can be a bit opaque what type consumers will actually see. Often the compiler will produce an any in the face of an error so the bad type doesn’t propagate and cause errors in other places, but it’s really not obvious that you have an any and what other types that any will infect.

i agree, i was thinking about this the other day actually, what if there was a compiler option to fallback to unknown instead of any on such errors #46330

Not quite—it’s perfectly “safe” to use one of those in implementation code that doesn’t contribute to the type signature of any globals or module exports. It’s only bad news when the comment applies to something that will necessarily be copied to the declaration files, like a return type annotation of an exported function or something.

would it be possible to have an error when using @ts-ignore and @ts-expect-error in one of these dangerous spots? #46331 (if not, i also raised typescript-eslint/typescript-eslint#3995)

@geoffreytools
Copy link

The use case I have for ts-ignore is to make it impossible for a user to fill in a type parameter which I use as a temporary variable, by making it extends never.

I think this use cases is legitimate as it is not an attempt to silence an error which will propagate and break things. It's working around a limitation in the language (in this case: that we don't have temporary type variables, but it could be something else) in a way that is, to the contrary, safer than the alternative which would be to not put a type constraint at all on this parameter.

I used this pattern twice in the same library, you will find extracts bellow but I also want to comment on the developer experience related to this issue.

Today I realised I was forced to compile my type-level libraries in order to make them compatible with TS Playground (and maybe other tools which don't seem to be willing to import .ts files directly). I was going through the process of doing it when I noticed that the compiled version was not strictly equivalent to the source. I was not even warned during compilation. It's a good thing I decided to test the dist folder instead of the src folder, but this is very impractical because it forces me to build before I test. I can also no longer benefit from the instant feedback VS Code was giving me with the language server. I believe users should be able to trust that the compiler does not change the behaviour of their code.

The instances in which I used the pattern:

type Pipe<
    Args extends $Ts[0]['constraints'],
    $Ts extends [Result] extends [never] ? never : Composition,
    // @ts-ignore: prevent users from messing with it
    Result extends never = _Pipe<Args, $Ts>
    //     -------------
> = Result

Pipe enables to compose types in a type-safe way, but the logic for checking the composition is the same as the one for actually doing the composition, so I stored the result in a temp variable as an optimisation.

I did something similar in this other type: in order to not repeat GetOtherwise<Cases> which is computationally intensive. This time it is not used to check the parameters of $Match directly (although it's eventually used for type checking as well)

interface $Match<
    Cases extends Case[] & CheckCases<Model, Cases>,
    Model extends CheckFallThrough<Model, Cases> = never,
    // @ts-ignore: prevent users from messing with it
    //        vvvvvvvvvvvvv
    Otherwise extends never = GetOtherwise<Cases>
> extends Type<1> {
    type: _Match<this[A], Cases>
    constraints: [
        [Otherwise] extends [never] ? unknown
        : Otherwise extends _never ? unknown
        : Otherwise extends Type ? Otherwise['constraints']
        : unknown
    ]
}

In this case I was able to rewrite it this way because $Match is an interface and I know __Otherwise should be opaque to users

interface $Match<
    Cases extends Case[] & CheckCases<Model, Cases>,
    Model extends CheckFallThrough<Model, Cases> = never,
> extends Type<1> {
    type: _Match<this[A], Cases>
    constraints: [
        [this['__Otherwise']] extends [never] ? unknown
        : this['__Otherwise'] extends _never ? unknown
        : this['__Otherwise'] extends Type ? this['__Otherwise']['constraints']
        : unknown
    ]
    __Otherwise: GetOtherwise<Cases>
}

@luisherranz
Copy link

I just find out that if you use a /** @ts-expect-error **/ comment instead of // @ts-expect-error, it is preserved in the d.ts file.

@DetachHead
Copy link
Contributor

interesting. looks like /** @ts-expect-error */ is preserved but /* @ts-expect-error */ is not, i guess because jsdocs are preserved but regular comments aren't

@geoffreytools
Copy link

JSDoc is used in tooltips.

Can we rely on this behaviour though? Is this an unforeseen conflict or did someone think "gee, if you use that syntax you really mean to keep that ts-ignore".

Because if it gets "fixed" under our feet it will silently break users.

@unicornware
Copy link

unicornware commented Feb 22, 2023

@andrewbranch @RyanCavanaugh

preserving // @ts-ignore is necessary for working with peer dependencies:

  Try `npm i --save-dev @types/node-fetch` if it exists or add a new declaration (.d.ts) file containing `declare module 'node-fetch';`

72     req?: import('node-fetch').RequestInit | undefined;
                    ~~~~~~~~~~~~

node_modules/@flex-development/mlly/dist/interfaces/options-get-source.d.mts:50:18 - error TS7016: Could not find a declaration file for module 'node-fetch'. '/Users/lex/Projects/FLDV/FLDV-P032/node_modules/node-fetch/lib/index.js' implicitly has an 'any' type.
  Try `npm i --save-dev @types/node-fetch` if it exists or add a new declaration (.d.ts) file containing `declare module 'node-fetch';`

50     req?: import('node-fetch').RequestInit | undefined;

// @ts-ignore preservation in declaration files would silence these errors. my intention is for req to be of type any if my users have not installed node-fetch, rather than force them to install node-fetch so their typechecks pass with "skipLibCheck": false. additionally, the jsdoc comment workaround is not viable for those of who are actually writing documentation in our docblocks.

unicornware added a commit to flex-development/mlly that referenced this issue Feb 22, 2023
- @ts-* comments are meaningful comments, but typescript does not preserve them 🙃🙄
- re-implemented 779cddf as build plugin
- microsoft/TypeScript#38628
- microsoft/TypeScript#38628 (comment)

Signed-off-by: Lexus Drumgold <unicornware@flexdevelopment.llc>
@miguel-leon
Copy link

If I add a @ts-ignore (for a type) to a source .ts file, I expect the generated .d.ts file to also have the same @ts-ignore. As simple as that. Philosophies of "what is the correct thing to do" aside.
It should not provide a broken declaration file nor it should rely on the consumer of the package to use skipLibCheck.

Funny thing, ChatGPT suggests you use "emitDeclarationComments": true in compilerOptions which would be a wonderful solution if it existed 🤣

alecgibson added a commit to reedsy/quill that referenced this issue Jun 28, 2023
At the moment, since `@ts-expect-error` [doesn't apply][1] to type
definitions, we get a downstream compilation error:

```
TS2416: Property 'addModule' in type 'BaseTheme' is not assignable to the same property in base type 'Theme'.
  Type '(name: string) => unknown' is not assignable to type '{ (name: "clipboard"): Clipboard; (name: "keyboard"): Keyboard; (name: "uploader"): Uploader; (name: "history"): History; (name: "selection"): Selection; (name: string): unknown; }'.
    Type '{}' is missing the following properties from type 'Clipboard': matchers, addMatcher, convert, convertHTML, and 8 more.

10     addModule(name: string): unknown;
```

This change updates the `Base` class to have the same signature as
for `addModule()` as its parent class.

[1]: microsoft/TypeScript#38628
alecgibson added a commit to reedsy/quill that referenced this issue Jun 28, 2023
At the moment, since `@ts-expect-error` [doesn't apply][1] to type
definitions, we get a downstream compilation error:

```
TS2416: Property 'addModule' in type 'BaseTheme' is not assignable to the same property in base type 'Theme'.
  Type '(name: string) => unknown' is not assignable to type '{ (name: "clipboard"): Clipboard; (name: "keyboard"): Keyboard; (name: "uploader"): Uploader; (name: "history"): History; (name: "selection"): Selection; (name: string): unknown; }'.
    Type '{}' is missing the following properties from type 'Clipboard': matchers, addMatcher, convert, convertHTML, and 8 more.

10     addModule(name: string): unknown;
```

This change updates the `Base` class to have the same signature as
for `addModule()` as its parent class.

[1]: microsoft/TypeScript#38628
alecgibson added a commit to reedsy/quill that referenced this issue Jun 28, 2023
At the moment, since `@ts-expect-error` [doesn't apply][1] to type
definitions, we get a downstream compilation error:

```
TS2416: Property 'listeners' in type 'Emitter' is not assignable to the same property in base type 'EventEmitter<string, any>'.
  Type 'Record<string, { node: Node; handler: Function; }[]>' is not assignable to type '<T extends string>(event: T) => ((...args: any[]) => void)[]'.
    Type 'Record<string, { node: Node; handler: Function; }[]>' provides no match for the signature '<T extends string>(event: T): ((...args: any[]) => void)[]'.

19     listeners: Record<string, {
```

This change removes the clash by renaming the internal variable. Note
that, since `listeners` is `public`, this is technically breaking.

[1]: microsoft/TypeScript#38628
luin pushed a commit to slab/quill that referenced this issue Jun 29, 2023
At the moment, since `@ts-expect-error` [doesn't apply][1] to type
definitions, we get a downstream compilation error:

```
TS2416: Property 'addModule' in type 'BaseTheme' is not assignable to the same property in base type 'Theme'.
  Type '(name: string) => unknown' is not assignable to type '{ (name: "clipboard"): Clipboard; (name: "keyboard"): Keyboard; (name: "uploader"): Uploader; (name: "history"): History; (name: "selection"): Selection; (name: string): unknown; }'.
    Type '{}' is missing the following properties from type 'Clipboard': matchers, addMatcher, convert, convertHTML, and 8 more.

10     addModule(name: string): unknown;
```

This change updates the `Base` class to have the same signature as
for `addModule()` as its parent class.

[1]: microsoft/TypeScript#38628
luin pushed a commit to cmrd-senya/quill that referenced this issue Aug 7, 2023
At the moment, since `@ts-expect-error` [doesn't apply][1] to type
definitions, we get a downstream compilation error:

```
TS2416: Property 'addModule' in type 'BaseTheme' is not assignable to the same property in base type 'Theme'.
  Type '(name: string) => unknown' is not assignable to type '{ (name: "clipboard"): Clipboard; (name: "keyboard"): Keyboard; (name: "uploader"): Uploader; (name: "history"): History; (name: "selection"): Selection; (name: string): unknown; }'.
    Type '{}' is missing the following properties from type 'Clipboard': matchers, addMatcher, convert, convertHTML, and 8 more.

10     addModule(name: string): unknown;
```

This change updates the `Base` class to have the same signature as
for `addModule()` as its parent class.

[1]: microsoft/TypeScript#38628
@mlcruz
Copy link

mlcruz commented Nov 24, 2023

Not being able to propagate @ts-ignore and so on to the generated .d.ts file makes it super annoying to deal with the kind of bug that you can see in the following issue (typescript works fine, but d.ts breaks).

colinhacks/zod#2260

Right now I'm trying to expose some shared zod type definitions to internal libs and i'd rather not use skipLibCheck. Being able to just add a few @ts-ignores so i can make my d.ts file work as a workaround would be much better.

@fabian-hiller
Copy link

fabian-hiller commented Aug 18, 2024

Same problem here. I thought // @ts-expect-error was preserved in the declaration files. We use // @ts-expect-error for recursive types, which are only used in a way where the recursive implementation is not an actual problem. I see no other way than to ignore the TS error.

Here is an example:

// Types ----------------------------------------------------------------------

interface BaseIssue<TInput> {
  type: string;
  input: TInput;
}

interface BaseSchema<TOutput, TIssue extends BaseIssue<unknown>> {
  type: string;
  _run: (input: unknown) => TOutput;
  output: TOutput;
  issue: TIssue;
}

interface StringIssue extends BaseIssue<string> {
  type: 'string';
}

interface StringSchema extends BaseSchema<string, StringIssue> {
  type: 'string';
}

interface NumberIssue extends BaseIssue<number> {
  type: 'number';
}

interface NumberSchema extends BaseSchema<number, NumberIssue> {
  type: 'number';
}

interface LazySchema<TSchema extends BaseSchema<unknown, BaseIssue<unknown>>>
  extends BaseSchema<TSchema['output'], TSchema['issue']> {
  type: 'lazy';
}

// Implementation ------------------------------------------------------------

// @ts-expect-error
type MaybeLazyString = StringSchema | LazySchema<MaybeLazyString>;

type IsMaybeLazyString<
  TSchema extends BaseSchema<unknown, BaseIssue<unknown>>,
> = TSchema extends MaybeLazyString ? true : false;

// Tests ---------------------------------------------------------------------

type Test1 = IsMaybeLazyString<LazySchema<LazySchema<StringSchema>>>; // -> true

type Test2 = IsMaybeLazyString<NumberSchema>; // -> false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests