mightyiam / eslint-config-love

A TypeScript ESLint config that loves you
MIT License
774 stars 69 forks source link

Rule clash between @typescript-eslint/init-declarations and no-undef-init #1710

Closed xandris closed 3 weeks ago

xandris commented 2 months ago

Given a stock config, local variables that may be undefined are impossible to lint:

// Variable 'x' should be initialized on declaration
// @typescript-eslint/init-declarations
let x: number | undefined

// It's not necessary to initialize 'x: number | undefined' to undefined
// no-undef-init
let x: number | undefined = undefined

The safer route is probably to turn off no-undef-init and allow superfluous initialization with undefined? At least in Typescript.

mightyiam commented 2 months ago

Sorry about this. And thank you for raising it. Will take a look. Perhaps this weekend.

xandris commented 2 months ago

I actually ended up turning off @typescript-eslint/init-declarations because for our codebase the problem it prevents is already caught by the Typescript compiler (TS2454 Variable is used before being assigned)

mightyiam commented 2 months ago

Thank you for the additional feedback, @xandris.

mightyiam commented 2 months ago

Analysis with @rostislav-simonik conclusion: Both rules are desirable, except that https://github.com/typescript-eslint/typescript-eslint/issues/9950.

@xandris would you concur?

We suggest waiting for upstream's response.

xandris commented 2 months ago

yes, i agree completely

mightyiam commented 2 months ago

https://github.com/typescript-eslint/typescript-eslint/issues/9950#issuecomment-2336558626

@xandris say... how many of these do you have? Could they possibly be nicer as null instead?

darkbasic commented 1 month ago

Upstream locked the issue so I think the lesser evil is to disable init-declarations. At least that's what I did.

xandris commented 1 month ago

https://github.com/typescript-eslint/typescript-eslint/issues/9950#issuecomment-2336558626

@xandris say... how many of these do you have? Could they possibly be nicer as null instead?

not many... it's usually like an initializer that's too complex for a ternary. the variables are not actually nullable; they really do get assigned before use and the compiler does enforce that

mightyiam commented 1 month ago

Thank you @darkbasic and @xandris .

"not many" leads me towards eslint-disable comments. Thoughts on that?

darkbasic commented 1 month ago

@mightyiam I use let myVar: MyType | undefined = undefined quite a lot in try/catches for example so I wouldn't be so fond of using eslint-disable comments.

mightyiam commented 3 weeks ago

@darkbasic could I possible trouble you to provide some example, please?

darkbasic commented 3 weeks ago

@mightyiam for example in the project I'm currently working right now I need to share some variables between different event listeners to make vuetify tables resizable. I need to create these variables upfront and I cannot initialize them with anything else but undefined:

image

mightyiam commented 3 weeks ago

@darkbasic why not null?

darkbasic commented 3 weeks ago

@mightyiam there are cases where null might be a valid value that could be set and there would be no way to tell if the variable has been assigned or not.

darkbasic commented 3 weeks ago

The problem is that until this will be properly fixed Typescript doesn't create a union with undefined for variables that have been declared but not assigned. So you should be forced to assign a value to the variable, even if it's just undefined.

mightyiam commented 3 weeks ago

@darkbasic perhaps it's appropriate in the case you mentioned to disable the rule ad-hoc for some limited scope, or even the entire project. See a recent readme section: https://github.com/mightyiam/eslint-config-love/blob/01af4999a739cb4ec8c51e67c63f0bf3431f5246/README.md#disabling-rules

I don't see a valid reason to disable the rule, though. Please try to prove me wrong if I'm wrong.

darkbasic commented 3 weeks ago

Let's try the other way around: what do you think is the value of no-undef-init? Because I don't see any and maybe that's skewing the way I weight its benefits.

I don't see a valid reason to disable the rule

The reason is that Typescript is not able to infer undefined from a variable that has not been assigned yet. So you have to be forced to add undefined to the union of the possible variable types to overcome that TS limitation. The only way to force the user to add undefined to the union is via init-declarations, but no-undef-init prevents you from doing so. I value init-declarations much more than no-undef-init for the reasons I've just explained.

darkbasic commented 3 weeks ago

This is from openapi-typescript documentation:

image

They do the same thing: they assign undefined to the variable as a way to force you to add undefined to its type in order to circumvent the Typescript limitations.

mightyiam commented 3 weeks ago

The value of no-undef-init is in choosing a style in cases where a choice is present. In cases where initializing with undefined and not initializing is equivalent, then this rule makes the decision for us. That seems to be the entirety of its value.

It seems that in TS, as opposed to in JS, some cases are not equivalent.

Is this correct?

darkbasic commented 3 weeks ago

It seems that in TS, as opposed to in JS, some cases are not equivalent.

There are non equivalent cases in JS as well: think of a loop where you are using a non block-scoping statement like var: the variable declaration will be hoisted outside of the loop while the assignment will remain inside the loop. So this rule goes beyond styling even with plain JS, but it gets worse with Typescript because it prevents you from always assigning a value to the variables that you declare. You should always assign a value (even if it's undefined) because Typescript is still not smart enough to understand if that variable has been initialized or not and thus change its type accordingly. Forcing you do to so means that you will manually add | undefined to its type circumventing this limitation. This has been fixed in the latest Typescript Nightlies but since it didn't make its way into a stable release yet I hadn't a chance to test it and I'm not sure if it covers every possible corner case. So we should still use init-declarations and thus disable no-undef-init for the moment.

mightyiam commented 3 weeks ago

Thank you, @darkbasic.

eslint-config-love-release-bot[bot] commented 3 weeks ago

:tada: This issue has been resolved in version 89.0.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: