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

Inline comments always stripped from .d.ts, including @ts-expect-error, @ts-ignore etc. #59365

Closed
ssalbdivad opened this issue Jul 19, 2024 · 9 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@ssalbdivad
Copy link

πŸ”Ž Search Terms

ts-expect-error ts-ignore removeComments .d.ts emit stripped

πŸ•— Version & Regression Information

This is the behavior in every version I tried

⏯ Playground Link

https://tsplay.dev/wjje8w

πŸ’» Code

const InnerDynamicBase = class {
	constructor(properties: object) {
		Object.assign(this, properties)
	}
} as new <t extends object>(base: t) => t

// this pattern allows adding properties to a class from a type parameter
// but requires ignoring an error internally for the declaration

// the first comment will be stripped from .d.ts emit, even with
// removeComments explicitly set to false in tsconfig

// consumers then end up with a type error in node_modules, which can break
// their build if they don't have skipLibCheck set to true

// @ts-expect-error
class DynamicBaseInline<t extends object> extends InnerDynamicBase<t> {}


/** @ts-expect-error **/
class DynamicBaseBlock<t extends object> extends InnerDynamicBase<t> {}


// doesn't show anything on hover (good)
DynamicBaseInline


// shows "@ts-expect-error β€” *" on hover (bad)
DynamicBaseBlock

πŸ™ Actual behavior

.d.ts output

declare const InnerDynamicBase: new <t extends object>(base: t) => t;
declare class DynamicBaseInline<t extends object> extends InnerDynamicBase<t> {}
/** @ts-expect-error **/
declare class DynamicBaseBlock<t extends object> extends InnerDynamicBase<t> {}

πŸ™‚ Expected behavior

.d.ts output

declare const InnerDynamicBase: new <t extends object>(base: t) => t;
// @ts-expect-error
declare class DynamicBaseInline<t extends object> extends InnerDynamicBase<t> {}
/** @ts-expect-error **/
declare class DynamicBaseBlock<t extends object> extends InnerDynamicBase<t> {}

Additional information about the issue

I expect the issue is probably determining whether certain comments "belong" to JS implementation or .d.ts, and it assumed that only block-style comments that appear as JSDoc should be long to the types.

I think there are likely other cases where comments on types should be preserved (e.g. a comment within a generic type implementation that is not emitted to JS), but at the very least, comments that affect type checking like @ts-expect-error and @ts-ignore should always be emitted.

@MartinJohns
Copy link
Contributor

@ts-expect-error and @ts-ignore should always be emitted.

These are intentionally not emitted. See #43570 / #38628.

@ssalbdivad
Copy link
Author

I really think this needs to be revisited.

  1. You can still enforce those comments in .d.ts with block style like /** @ts-expect-error **/, but it looks awful in JSDoc
  2. There are cases like the one I mentioned and "casting" variance annotations where there is no other method of working around the error
  3. skipLibCheck is still off by default, meaning this can very easily break downstream builds

@RyanCavanaugh mentioned:

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

But silencing the error at the use site is all I want to do. I'm well aware it doesn't affect downstream types- that's exactly the point. The downstream types are already correct, I just don't want users having their builds broken because I or some other author used // instead of /** **/.

@andrewbranch Are you aware of workarounds for cases like those I mentioned or open to reconsidering this behavior?

@jakebailey
Copy link
Member

We are already unhappy with people using ts-ignore and then trying to use whatever resulted from that operation. I think explicitly letting that happen in d.ts files is even worse...

Why do you need to do this? Why is error-free code not viable here?

@ssalbdivad
Copy link
Author

I understand by doing this I'm opting into "unsafe" behavior that could break in a future version of TS, but all of my types are tested and the only thing should matter for an external consumer is that the output is correct, not whether my implementation in some environment is determined to include an error.

As mentioned, there are a few cases where as far as I'm aware, the only way to get the type-level behavior desired is with internal code that requires some ignore comment. For example, TS occasionally can't identify that it is safe to treat a class as covariant due to some conditionals that it fails to precisely evaluate, but I can overwrite the annotation so downstream users get the correct behavior by manually annotating it with out and then ignoring it. In terms of safety, it is fundamentally no different than any other cast; I'm just prompting TS that I know something is safe to treat a certain way and to trust my judgment.

To be honest though, I think it is probably true that 95%+ of the time that // @ts-ignore or // @ts-expect-error are used outside a test, it's a mistake. Casting or performing additional checks or intersections would be the better approach. So I understand why you want people to avoid these comments.

That said, I don't believe that stripping them from .d.ts output is actually helping anyone realize when they're being used incorrectly. It's just creating potential downstream problems for consumers of those types that have nothing to do with the output of the types themselves.

The behavior intended by the person publishing the types is clearly whatever behavior they're already seeing locally, with the comment added.

No matter how bad you think these comments are, emitting .d.ts that is semantically different (i.e. can lead to different build results) does not feel like a solution.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jul 26, 2024
@RyanCavanaugh
Copy link
Member

This just isn't a supported use of ts-ignore and we're not going to open the enormous can of worms it would be to correctly detect when these comments should/shouldn't be in the output. It's even more fraught for something like ts-expect-error where an error might be conditional on compiler settings. If you are truly breaking the type system in a way that you fully understand, putting up the high bar that you need to post-process your output seems entirely appropriate.

@ssalbdivad
Copy link
Author

I know that I can work around this using block-style comments, so no post-process is required (as long as I'm willing to put up with some mildly wonky JSDoc suffixes on hover).

My main concern is that regardless of how necessary for most people to employ these comments, the reality is that they're very common, and having them stripped out unless they're written as block comments does not feel intuitive or beneficial to anyone involved.

If it's just not a priority I can understand that, but I just don't see any upside to potentially exposing downstream errors that were intentionally suppressed in library code.

@MartinJohns
Copy link
Contributor

having them stripped out unless they're written as block comments does not feel intuitive or beneficial to anyone involved.

That they're not removed when writing /** @ts-expect-error **/ seems like a bug to me.

@ssalbdivad
Copy link
Author

@MartinJohns I agree that this is an inconsistency, so the question then becomes how to resolve it. I still can't wrap my head around who it benefits to strip these out.

@typescript-bot
Copy link
Collaborator

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

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 30, 2024
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

5 participants