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

Support @ts-ignore for specific errors #19139

Open
andy-ms opened this issue Oct 12, 2017 · 95 comments
Open

Support @ts-ignore for specific errors #19139

andy-ms opened this issue Oct 12, 2017 · 95 comments
Labels
Revisit An issue worth coming back to Suggestion An idea for TypeScript

Comments

@andy-ms
Copy link
Contributor

andy-ms commented Oct 12, 2017

TypeScript Version: 2.6.0-dev.20171011

Code

function countDown(n: number): void {
    switch (n) {
        // @ts-ignore
        case 1:
            console.log("1");
            // intentional fall through
        case 0:
            console.log("0");
    }
}

Expected behavior:

Ability to make ts-ignore apply to the --noFallthroughCasesInSwitch error but not to other errors.

Actual behavior:

case "1": would also compile.

@megan-starr9
Copy link

megan-starr9 commented Oct 12, 2017

Seconding (and wanting to track this)

This would be super helpful in our case as we have a class with a bunch of variables, and a function that accesses them by key value (vs direct getter function). As a result, they are never called directly within the class and are throwing the error despite being necessary to the functionality. I don't want to have to disable the "noUnusedLocals" check across the board, just in the two classes where this is the case.

Edit -
It's been 2 years and a job change, but I wanted to clarify something bc I'm looking back at old issues and realized this description is slightly unhelpful
My use case for this request was more along the lines of this introduced feature
#33383
But I imagine I did not want to ignore every rule in the file, so wanted to be able to specifically turn off the rule that wasn't preventing working code in an edge case, but we didn't want becoming common practice through the code base

@Ristaaf
Copy link

Ristaaf commented Nov 28, 2017

I would also love this, more specifically for the noUnusedLocals rule, we get errors on this:

class NumberEvaluator {
   private expression;
   ...
   public evaluate(){
      this[this.expression.type]();
   }

   private BinaryExpression() {
      ...
   }
   ...
}

Saying that BinaryExpression is not used, but it might be, depeding on the value of this.expression.type. I could make BinaryExpresison public, but I really think it is a private method.

@alexburner
Copy link

alexburner commented Dec 13, 2017

This would also be very useful for our team, we are slowly migrating a large JS app to TS. We'd like to turn on strict compilation flags (especially --noImplicitAny and --strictNullChecks) but have lots of deviant files that need fixing.

Specific error suppression would allow us to gradually migrate everything to strict compilation.

@stweedie
Copy link

It's actually absurd that this is even still an issue. I've experienced a problem related to this. The underlying cause was due to a typescript change in 2.4, as discussed here:

ag-grid/ag-grid#1708

We were then left with three options

  1. pin typescript to some version < 2.4
  2. update our code to allow updating ag grid to a compliant version
  3. fork our own version of ag-grid until we can upgrade

option 1 is not preferable, as it means we can't leverage any new typescript features and can result in having mismatched dependencies

options 2 and 3 are possible (although difficult), and they really only handle one such specific 'error' preventing successful compilation.

Why is there not an option for us to 'ignore' TS2559 (as an example)?

@Busyrev
Copy link
Contributor

Busyrev commented Feb 15, 2018

@stweedie I made pull request with ability to specify error code to ignore, #21602 but nothing happens,
@andy-ms What you think about?

@thw0rted
Copy link

@Ristaaf I know it's been a while, but I think your use case (private but only referenced dynamically) could use the @internal decorator, like /** @internal */ BinaryExpression() { ... }. Technically your method is public so tsc won't complain but it doesn't show up in the docs as being part of your API. (I can't seem to find any docs on @internal but I know it's used extensively in Angular...)

@wosevision
Copy link

Here's another little doozy that I don't believe is covered by any "normal" TS methods. Consider the following:

You're dynamically creating a web worker, i.e. by using an ObjectURL created from a Blob containing the .toString()-ified contents of a function:

const objectUrl = URL.createObjectURL(
  new Blob(
    [`(${
      function() { ... }.toString()
    })()`],
    { type: 'application/javascript' }
  )
);

...and the body of that function contains the single most important thing a web worker can do, postMessage():

// inside worker function
setInterval(() => postMessage('from worker!'), 2000);
                              // ^^^ this causes TS error! 😿

Oh no! TypeScript's implementation of postMessage has a required target parameter in it's function signature. Problem is, we're not using window.postMessage, we're using **self**.postMessage, i.e. the web worker's context! Furthermore, trying to spoof it with a null value makes it worse:

// inside worker function
setInterval(() => postMessage('from worker!', null), 2000);
                              // ^^^ this causes worker exception! ☠️

The web worker postMessage won't accept another parameter! Woe is me.

Since it doesn't make sense to change the [correct] type definition for the window context's interface, and web workers can't use TypeScript [natively], it seems this would be a perfect opportunity to tell the TS compiler "Hey dude, that's a stringified version of some arbitrary Javascript that has nothing to do with you, your execution context even. Back off, get your own sandwich".

@HolgerJeromin
Copy link
Contributor

@wosevision
Webworker have a special API which is covered by lib.webworker.d.ts
After adding the target Webworker to your tsconfig you should be able to have a fully typed code like this:
setInterval((this: BroadcastChannel) => this.postMessage('from worker!'), 2000);

disclaimer: I never worked with Webworker till now.
For further question opening a stackoverflow question is probably the best.

@wosevision
Copy link

@HolgerJeromin Thanks, I appreciate the tip. I was aware of the TS webworker lib and target, but this situation is slightly different. The linchpin point is here:

dynamically creating a web worker, i.e. by using an ObjectURL created from a Blob

...That is to say, the target is not a webworker. It's just regular browser JS that happens to be building the webworker. The lib should still reflect the regular lib.dom.d.ts because, for all intents and purposes, we are not inside a webworker and therefore lib.webworker.d.ts typings are useless everywhere else.

It's such an edge case that it would feel silly to do anything other than ignore a line or two.

@jlengrand
Copy link

Is there a reason this issue is still open? I depend on a library that has an incorrect documentation, and it causes unsilencable errors!

See esteban-uo/picasa#27 for more info. This feels a bit silly.

@mjomble
Copy link

mjomble commented Jul 12, 2018

@jlengrand you could use a generic // @ts-ignore comment.
It already works and ignores all TypeScript errors on the next line.
This issue is only about enhancing the feature to enable targeting more specific errors.

@jlengrand
Copy link

Ha nice, I didn't understand that! I saw the ts-ignore issue closed, thinking it had been dismissed. Thanks for the tip!

@ghost
Copy link

ghost commented Sep 13, 2018

Without fine control about how legacy files are handled it is really hard to introduce new typescript rules into a project.

Please give this use case some more consideration.

@ilyakatz
Copy link

👍

@macdaj
Copy link

macdaj commented Dec 11, 2020

@RyanCavanaugh
I get the perspective of backwards compatibility and maintenance of old code being a burden.

In my specific case I'm running into a problem related to immer losing symbol keys which I believe is caused by keyof removing well known symbols. For this case I have to use a ts-ignore statement. I think adding some compiler options would really help because ideally I want to remove these ts-ignore lines once they are no longer needed, or in a perfect world pay attention to different ts exceptions that pop up on the ignored line.

I think for ts-ignore having an explicit opt-in typescript compiler option that would throw an error if there is no typescript error on the line would be useful.

I think the same approach where specific error ts-expect-error-#### statements would fall back to the general ts-ignore would work. You can have a compiler option which specifies whether you want the fallback (by default it is true), effectively making ts-expect-error-#### be an explicit opt-in.

Even if you don't opt into either of these behaviors you have the ability to periodically run a build with these rather than checking each line manually.

@ghost
Copy link

ghost commented Jan 14, 2021

Personally, I find entire TSC errors to be, without explicit rationale behind them, 100% useless.
Consider TS18022, why is that even an error?
Or TS2775.
Or TS18019.
I legitimately had made a class that used a static, private class method, that asserted and determined whether or not something was genuinely from that class.

Oops, I broke at least 3 TS rules at once :)
No static private, no private methods, no assertion object methods.
None of these are poor coding practice, they aren't inherently unsafe, they can be statically analyzed, but they are denied by TS, which prevented me from porting my JS to TS.

There are plenty of errors that are just that, errors, they should make someone stop and think about whether what they had just written was unsafe, and rewrite it, but the "errors" that I would like to disable en masse fall into a whole different category outside of safety and linting, into the area of "why is that an error in the first place?"

@thw0rted
Copy link

For both TS18022 and TS18019, your linked issue points out that private methods only reached "Stage 3" quite recently and TS hasn't gotten around to supporting them yet. Likewise for other up-and-coming ES private features (static #foo, etc). The good news is that TS legend and all-around swell guy @dragomirtitian says in #39066 that support is inbound soon.

For TS2775, the linked issue (and a PR that is linked several times from there) make it pretty clear that there's a design limitation for control-flow analysis where you have to basically "re-type" the assert function when it's imported. (I agree with what appears to be the general consensus that the error message could be clearer, but at least searching for that error number brings you to those discussions.) This is irksome but not insurmountable.

@ghost
Copy link

ghost commented Jan 14, 2021

For TS2775, the linked issue (and a PR that is linked several times from there) make it pretty clear that there's a design limitation for control-flow analysis where you have to basically "re-type" the assert function when it's imported. (I agree with what appears to be the general consensus that the error message could be clearer, but at least searching for that error number brings you to those discussions.) This is irksome but not insurmountable.

I think I'm experiencing a different error now that I look at it; I already know that reassigning an assertion function requires an explicit type, but what about direct calls?

let azzert = new Assertion().assert;
azzert(foo);

vs

new Assertion().assert(foo);

my particular case looked more like:

class T {
    static #assertIsT(x: unknown): asserts x is T { ... } // error

    method(x: unknown) {
        T.#assertIsT(x); // error
        ...
    }
}

but the real problem here is the lack of static private support and is the only reason that it can't be typed correctly.

For both TS18022 and TS18019, your linked issue points out that private methods only reached "Stage 3" quite recently

Well... those issues have actually been open for quite some time, 10 and 11 months respectively.
That means in a month or two, private methods and private statics will have been at stage 3 for at least a year.

But, in reality, the numbers are a bit different:

Proposal name Date that it reached stage 3
proposal private methods Sepetember 2017
proposal static class features May 2018
proposal class fields July 2017

An average of three years since they reached stage 3; these are bound to reach stage 4 soon.
And, to top it off, Chrome already ships a full implementation, by default (e.g. no experimental flags).

Also, there are errors like TS2376 and TS17009, which seem... like they fulfill the same role, why does one exist, if the other does?

class T extends H {
     foo = ...;

    constructor() {
        const x = bar();
        super(x, x); // TS#### : "don't do that"
        // do I do this? // super( bar(), bar() );
    }
}

@soullivaneuh
Copy link

I am not a Typescript expert, so I'll not add arguments of the previous debate.

However, I would like to express the need of this granularity with an example.

I use react-navigation that allows us to declare routes with Typescript:

export type MyStackParamList = {
  Index: undefined;
  Show: {
    id?: string;
  };
  Edit: {
    id: string;
  };
};
const Stack = createStackNavigator<RidesStackParamList>();

You may define multiple stacks like that, but I will not going to details here.

Using the navigation to change the current route is supposed to be limited to the current stack, so if I do:

navigation.navigate('Payment');

Typescript will throw error TS2345 because the Payment parameter does not match the related MyStackParamList but an another stack.

However, this code part works like a charm. You can navigate to an another route of an another stack without issue

Is it me wrongly using the library? Maybe, surely. It's classified.

However, supposing it's an issue related to the vendor type definition, I can do anything waiting the fix than ignoring the error.

But as @andy-ms said, I would like to be sure only this error will be silenced and avoid possible other mistakes on the same line.

If specifying errors to ignore is dangerous according to some people here, what are the alternative for this case?

Thanks for reading

@arthur-clifford
Copy link

The only actual argument, if I'm not mistaken, is the instability of the error codes.

A couple thoughts

  1. I really hope your team did something to standardize error codes between versions; the fact that there was any resistance to that idea is itself disturbing. It is the more forward compatible option that gives the most flexibility in the long run even if you end up with a bunch of unused codes. It may be hard, but who cares about hard; if we wanted easy we'd all be using square space and not coding our own stuff in typescript.
  2. There have been very compelling arguments for the utility of refined expect-error or ts-ignore entries.
  3. noIplicitAny and other tsconfig linter options are already examples of this capacity except they use names rather than codes; the advantage of the codes is that they can be linked to definitions that are internationalized.

All we are asking for is the ability to ignore those errors that we deem to be more style than error especially when the code actually runs, yet not have to not turn everything off.

The general reply in effect is that it is a bad idea to turn off errors but if you have to then turn them all off which makes no sense at all whether at the global or individual line level.

What would the actual level of effort be to implement ignorable/expectable error codes at the comment AND tsconfig level for consistency?

If it were implemented, would the folks in this thread be ok with the ts compiler generating a report of ignored errors that could in some way (via opt-in program) reported back to the ts dev team so that they can have some real world feedback on what all is getting ignored?

My problem with suggestions like "we are not seeing issues with ..." is that you aren't looking at any real code. You are waiting for the rarified people who know enough to complain, and with enough time to, to actually complain. To make those kind of arguments you need real feedback but if you are making it so we can't know what the errors we're ignoring are, except all of them, then how can you possibly know that your errors are useful or what the consequences of your choices are?

I'm thinking there is an opportunity for a meaningful feedback loop and a win-win if you let us codify our overrides inline with some metric info going back to you rather than have to come here to complain about things,

// @ts-ignore TS/1234 because I know what I'm doing
Seems a lot more reasonable than
// @ts-ignore developers
Which leads to
// @ts-expect-error typescript devs do not care so neither should we

@doberkofler
Copy link

@arthur-clifford could not agree more

@nojvek
Copy link
Contributor

nojvek commented Jun 29, 2021

The other big argument is error code specific ignores on a single line.

Suppose you want to ignore two specific errors on the same line, you should be able to // @ts-expect-error TS123, TS2345 only those two kinds of errors. If at a future date, a new error pops up on the same line but of a different kind, you don't want to have that ignored.

Having a more granular "hold the line" functionality would be a massive boon.

@rluvaton
Copy link

rluvaton commented Sep 11, 2021

I'm really looking to this feature and it really important IMO

(I'm sorry if it's already been said...)

[...] Suppose you want to ignore two specific errors on the same line, you should be able to // @ts-expect-error TS123, TS2345 only those two kinds of errors. [...]

The problem with using error code numbers (e.g. TS123) is that the reader is not able to understand from the code what the error we're expecting, I think an ESLint type of ignoring errors would be much more user friendly (e.g. // eslint-disable-next-line no-alert, quotes, semi)

@hsalkaline
Copy link

Another case where disabling specific error is useful is refactoring large codebase.
Imagine we want to set noUncheckedIndexedAccess to true in our tsconfig. And turning this flag on reveals ~1.5k errors. Fixing that much errors in one PR is hard and dangerous. Possible solution is to mark all errors occurrences with @ts-expect-error with specific code (which is safe to do and can by done with a script, not manually) and then refactor errors iteratively.

@somebody1234
Copy link

somebody1234 commented Dec 26, 2021

i know im in a tiny minority here, but i (try to) have some sanity types to enforce relations (?) between various variables/types etc.

since they have zero use, they always trigger ts(6196) aka type declared but never used.
and of course, they're there so they throw a type error if the relation is violated. given the yellow highlight of the warning, it makes it extremely difficult to figure out whether i've fixed all the useful warnings.

@benkeen
Copy link

benkeen commented Feb 2, 2022

Adding my voice to the choir here. I inherited an old, large TS codebase where at some point someone decided to suppress the errors in the webpack build. With > 4,000 TS errors that spring up after fixing it, a single "Big Bang" PR to fix them all is unrealistic. I assumed I could just write a script to funnel the newly-identified errors identified from the linter into a script to add file-level @ts-ignore's, thus suppressing the specific errors in each file, then we could tackle the monstrous job of fixing all the problems piecemeal afterwards. But without the option to ts-ignore specific errors I find myself scratching my head...

@joebowbeer
Copy link

joebowbeer commented Feb 3, 2022

@benkeen to be clear, neither @ts-ignore or @ts-expect-error operate at the file level. @ts-nocheck does.

If @ts-expect-error accepted one or more tsErrno tags, as so many of us are hoping for, then you could parse the eslint output and precede every offending line with an expect-error comment.

By the way, not to make light of your situation, but disabling all the strict checking might reduce your issue to one that is easier to manage.

@langri-sha
Copy link

I really like that Flow forces me to ignore specific errors:

< // $FlowFixMe[incompatible-variance]
< // $FlowFixMe[incompatible-type]
< // $FlowFixMe[prop-missing]
> // @ts-ignore: Uhh.
foo(bar)

Slightly unrelated, but would be better if errors could match offending lines more closely:

> // @ts-ignore: Can't assign number.
process.env = {
  FOO: 'bar',
<  // $FlowFixMe[incompatible-type]  
  QUUX: 1000,
}

@stdedos
Copy link

stdedos commented Mar 30, 2022

For this feature, if it wasn't thought/said before, I would be expecting something like
// @ts-ignore TS1801 to ignore only a specific warning.

I also don't like the @ts-expect-error wording; it feels like I am writing this in tests. I'd prefer, instead, to have an option to make @ts-ignores act like @ts-expect-error instead.

@JonasDoe
Copy link

Another use case where I actually want warnings, but not all of them:

export function notEmpty<TValue>(
  value: TValue | null | undefined
): value is TValue {
  if (value == null) return false;
  const testDummy: TValue = value; // for compile warning
  return true;
}

I want a compile warning for the type check, but I don't want a warning for it being unused. Yes, I can add a line like testDummy = testDummy to make the compiler happy, but 1) that's an extra operation, 2) it bloats the code (a bit) and 3) my IDE is now complaining about the self-assignment.

@thw0rted
Copy link

Jonas, what you want is a "type expectation". There are a number of libraries that provide an expectType function or a comment-based directive to generate an error when the type of the passed/decorated variable is not what you wanted. Look at

https://github.com/microsoft/dtslint
https://github.com/SamVerschueren/tsd
https://www.npmjs.com/package/eslint-plugin-expect-type?activeTab=readme

etc

@sarimarton
Copy link

I'm repeatedly coming back to this issue. @ts-expect-error doesn't make sense without specifying the error. I don't mind that @ts-ignore can't be narrowed down, eslint's default rules prohibit @ts-ignore anyway. The problem with @ts-ignore is the implicit cleanup condition. But @ts-expect-error makes a lot of sense a lot of times with narrowing it down to one specific error. Eslint expects me to add a comment as well. But expecting any error makes the flag unsafe. So what's the point then?

Having said this, all the places where I see a reaction from a TS representative, I see wrong arguing. Focusing on @ts-ignore, bringing in short names (why?)...

"We don't want people to get into a situation where upgrading from TS X.Y to X.Y+1 yields hundreds of new errors simply because we changed an error message code"

I see 3 problems with this (at least):

  1. It's arguing with extremes. Having hundreds of @ts-expect-error on one particular error type in a codebase feels like a fishy case, but quite uncommon at least. And even then, I don't see a problem with search-and-replacing those comments. Also how common is that TS changes an error code for the same error condition? Has it even ever happened?

  2. Also, narrowing down @ts-expect-error is the developer's responsibility with all the consequences.

  3. It's indeed counterintuitive that error codes are tied to the error text. Why is that? Nobody expects that errors are defined by their text. Everybody treats the text as prone to being changed over time. That's why there's a code. On the other hand, introducing a new code for the same error already adds inconsistency to the global TS knowledge - think about googling new error codes not matching the results for the old one; and then maintaining a code history on all the blog posts and help pages on the internet, which explains a particular error...

@ThiagoMaia1
Copy link

Following

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Revisit An issue worth coming back to Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests