Skip to content

tsignore comment stripped from definition file #29866

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

Closed
MicahZoltu opened this issue Feb 11, 2019 · 6 comments
Closed

tsignore comment stripped from definition file #29866

MicahZoltu opened this issue Feb 11, 2019 · 6 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@MicahZoltu
Copy link
Contributor

TypeScript Version: 3.4.0-dev.20190209

Search Terms:
tsignore definition
tsignore declaration

Code

{
    "compilerOptions": {
        "removeComments": false,
        "declaration": true,
    }
}
//https://github.com/Microsoft/TypeScript/issues/202#issuecomment-429578149
// @ts-ignore
export type Branded<T, B> = infer _ extends B ? T : never

Expected behavior:
Generates a .d.ts file like this:

// @ts-ignore
export declare type Branded<T, B> = infer _ extends B ? T : never;

Actual behavior:
Generates a .d.ts file like this:

export declare type Branded<T, B> = infer _ extends B ? T : never;

While I recognize that this is some sketchy code, the important bit here is that the generated .d.ts file is invalid and will not compile, despite the .ts file it is generated from compiling. Either tsignore functionality should be removed entirely, or the @tsignore comments should make it into the generated declaration file when the ignored code makes it in.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Feb 19, 2019
@RyanCavanaugh
Copy link
Member

Part of the implicit contract of tsignore is that you not abuse it to suppress what are effectively syntax errors. Pushing a new syntax error into a .d.ts file is just making the problem worse.

While I recognize that this is some sketchy code, the important bit here is that the generated .d.ts file is invalid and will not compile, despite the .ts file it is generated from compiling. Either tsignore functionality should be removed entirely

No.

@MicahZoltu
Copy link
Contributor Author

That seems to argue that @tsignore should not be supported? Or are you saying that tsignore is only allowed if you keep it contained inside your library and don't let it leak out to effect other projects?


What would be the appropriate way to lobby for this syntax working without a tsignore? It works and serves a purpose in the type checker, and is even referenced as a workaround for TypeScript's lack of nominal types (the prevailing argument against adding nominal types to the language being that adding them is not necessary since workarounds like this exist).

@dragomirtitian
Copy link
Contributor

@MicahZoltu relying on a compiler error to get branded types seems like a horrible idea from the start. Why not use a more traditional approach to branded types. Just one way that keeps the brand field hidden would be:

class Brand<T> { private __brand!: T}
export type Branded<T, B> =T & Brand<B>


type Id1 = Branded<string, "id1">;
type Id2 = Branded<string, "id2">;
declare var id1: Id1, id2: Id2;


id1 = "foo" as Id1; // OK with cast
id1 = id1; // OK without cast

id1 = "foo"; // Type '"foo"' is not assignable to type 'Brand<string, "id1">'.
id1 = id2; // Type 'Brand<string, "id2">' is not assignable to type 'Brand<string, "id1">'.
id1 as unknown as Id2; 

@MicahZoltu
Copy link
Contributor Author

I can try that method again later to remind myself what about it sat poorly with me. IIRC one of the mechanisms (maybe this one) had the problem that it's error messages were significantly worse than the proposed solution. This solution is nice because it gives clear, succinct, one-line error messages.

@dragomirtitian
Copy link
Contributor

@MicahZoltu Sure the errors are one liners but you end up paying a lot more for that. Just as a comparison for the code I posted above:

The original errors:

id1 = "foo";
//Type '"foo"' is not assignable to type 'Branded<string, "id1">'
id1 = id2;
// Type 'Branded<string, "id2">' is not assignable to type 'Branded<string, "id1">'.

For my version:

id1 = "foo";
// Type '"foo"' is not assignable to type 'Branded<string, "id1">'.
//   Type '"foo"' is not assignable to type 'Brand<"id1">'.
id1 = id2;
// Type 'Branded<string, "id2">' is not assignable to type 'Branded<string, "id1">'.
//   Type 'Branded<string, "id2">' is not assignable to type 'Brand<"id1">'.
//     Types of property '__brand' are incompatible.
//       Type '"id2"' is not assignable to type '"id1"'.

While I will grand you they are not one liners the first line is exactly the same, I always read the first line and try to understand it before going in deeper as that is usually enough, the rest of the information is there just in case I need more (which in this case should never be necessary)

@Val-istar-Guo
Copy link

/** @ts-ignore */  <-- not removed
// @ts-ingore  <-- will be remove
/* @ts-ignore */ <-- will be remove

// and , /* */ ` not work????
typescript version: 4.0.3

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

4 participants