feat: upgrade plugin to v2.3.3

[mightyiam]

Closes #154.

[mightyiam]

I'm happy with this.

[mightyiam]

https://github.com/typescript-eslint/typescript-eslint/blob/v2.3.3/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md

[mightyiam]

@swansontec would you like to review?

[toddbluhm]

Yeah, this sounds like a really useful rule! I'm for it 👍

[swansontec]

This new rule generally makes sense, but there are some edge cases where it can get in the way, like unit testing or sanitizing incoming data. If you are writing a library designed for external consumption, you might do something like this:

function doMath (x: number): number {
  if (typeof x !== 'number') throw new TypeError('We only support numbers')
  return x * 2
}

The runtime check is redundant with the Typescript types, but it exists for the benefit of any Vanilla Javascript users. The only way to keep this code working is to switch to an assertion library, like assert(typeof x === 'number'), since that hides the check from ESLint. Since a workaround does exist for cases like this, this rule is not as bad as it could be, and I do see how it may have code-quality benefits in other places.

[LinusU]

Hmmm, assert won't throw TypeError though which is a bit of shame 🤔

Not really sure what my opinion is on this 🤔

[toddbluhm]

@swansontec You make a very valid point.

I wonder if there is any way to get typescript to generate those type assertions automatically when compiling down to vanilla js? Probably not, but that would be an interesting project to help improve vanilla js type-safety.

Edit: Looks like a package already exists for this: https://github.com/fabiandev/ts-runtime

[mightyiam]

So we have a rule here that in TypeScript tells you, "hey, don't check this type at run-time because according to type checking it's what you expect".

This is great, because it lets you know that you made a wrong assumption about what that value could be. So we'd like to have this.

But, if your code happens to be imported and used sans TypeScript, then you might decide that it's appropriate to type-check at run-time.

I've heard some developers call this "defensive programming". Is it dichotomous or is there a balance to run-time type checking sans static type checking?

If a library expects a string and is provided a number, should it throw with a nice error message or should it throw "foo.match is not a function" or silently behave unexpectedly?

Is it acceptable to enable this rule and use eslint-disable-line comments to explicitly declare "hey, I know that in TypeScript this check here is unnecessary, but this could be used sans TypeScript"?Perhaps that kind of lint-disable comment is appropriate in such a situation, to acknowledge to the reader that this is not an error, but intentional?

[swansontec]

So we have a rule here that in TypeScript tells you, "hey, don't check this type at run-time because according to type checking it's what you expect".

This is great, because it lets you know that you made a wrong assumption about what that value could be. So we'd like to have this.

I agree that this rule is helpful in a lot of cases (where you have strong type information), but not all projects have these guarantees. Enabling this rule could really make life difficult for those projects, so I don't think it is the kind of clean win that belongs in Standard.js.

The reason Standard.js can get away with being non-configurable is that it takes a generous "one-size-fits-all" approach. Anybody can enable Standard.js on their project and be happy; Standard.js doesn't block any legitimate coding patterns (at least, none that I have noticed in my 4 years of using it). The trade-off is that Standard.js doesn't enforce as many "best practices" as it could.

For instance, Standard.js doesn't include the no-var rule, even though it makes a lot of sense in modern Javascript. This is because writing "old-school" Javascript is still a valid use-case, and Standard.js doesn't want to prevent people from doing that. Given a choice between excluding people and enforcing a "best practice", Standard.js chooses to be more inclusive, and therefore more useful to more people. I believe the same principle should apply here.

[mightyiam]

Thank you for the input, @LinusU, @toddbluhm and @swansontec. I've deferred @typescript-eslint/no-unnecessary-condition #180 so that we could move on. This PR now does not add any rules.