eslint-config-love icon indicating copy to clipboard operation
eslint-config-love copied to clipboard

Resolve conflict between `prefer-optional-chain` and `strict-boolean-expressions`

Open gamesaucer opened this issue 2 years ago • 1 comments

What version of this package are you using?
21.0.1, installed as a dependency of ts-standard 11.0.0

What problem do you want to solve?
The rule prefer-optional-chain conflicts with the rule strict-boolean-expressions when using the strictNullChecks compiler option. This leads to the following scenario when using the default configuration:

let example: Set<number> | undefined

// Incorrect
if (example?.has(10)) {
  // ...
}

// Also incorrect
if (example !== undefined && example.has(10)) {
  // ...
}

// Correct
if (example !== undefined ? example.has(10) : false) {
  // ...
}

// Also correct
if (example !== undefined) {
  if (example.has(10)) {
    // ...
  }
}

// Also correct
if (example?.has(10) as boolean) {
  // ...
}

The reason for forbidding the first is that it doesn't "handle the nullish case explicitly". The reason for forbidding the second is that using an optional chain expression is "more concise and easier to read". Which of the two is it? You can't have both.

What do you think is the correct solution to this problem?
All the solutions that the linter allows are less concise and readable as a direct result of recommending the prefer-optional-chain rule, which is the opposite of its stated goal. It also directly conflicts with strict-boolean-expressions which is a rule that can actually work to catch bugs with unexpected nullish values and is not just for the sake of legibility, and which is useful outside of cases where it's possible to use optional chaining. It's my opinion that strict-boolean-expressions is the more valuable of the two rules by far.

Alternatively, strict-boolean-expressions can be configured not to apply to nullable objects which would allow both rules to exist in some form, but this makes the behaviour of strict-boolean-expressions inconsistent. I value consistency over being concise, so I don't like this compromise, but I still wanted to mention it since it is a solution to the problem.

Are you willing to submit a pull request to implement this change?
Doesn't seem like a big code change, it's more of an administrative thing. I'd rather leave it to someone who knows the package better.

gamesaucer avatar Jul 27 '22 21:07 gamesaucer

Yet another way to write this (that should be accepted by the linter) is:

if (example?.has(10) === true) {
   // ...
}

which is what I usually end up doing, although then my IDE recommends to simplify it. It's an endless cycle.

meyfa avatar Jul 28 '22 09:07 meyfa

Thank you for taking the time to share your concern, @gamesaucer . As @meyfa demonstrated, there is a rather concise expression that solves the conflict. And it seems rather correct to me and @rostislav-simonik . So we'll close this issue with this.

mightyiam avatar Nov 15 '22 07:11 mightyiam

// Also incorrect
if (example !== undefined && example.has(10)) {
  // ...
}

Is this really incorrect? That seems like a clear bug to me.


My recommendation is to do:

if (example?.has(10) ?? false) {
  // ...
}

I like this rule because it forces you to clearly specify what to do in the null case, and where it really shines is where you have some variable that is boolean | null.

LinusU avatar Nov 15 '22 09:11 LinusU