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

Enhancement: [strict-boolean-expressions] Change == to ===? #6173

Open
4 tasks done
justinhelmer opened this issue Dec 6, 2022 · 5 comments
Open
4 tasks done

Enhancement: [strict-boolean-expressions] Change == to ===? #6173

justinhelmer opened this issue Dec 6, 2022 · 5 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@justinhelmer
Copy link

justinhelmer commented Dec 6, 2022

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/strict-boolean-expressions/

Description

Hello! We believe strongly that strict boolean expressions are a good thing, because truthy evaluation opens the door for developer error and bugs to slip into the wild.

For this reason, we very much like the power of the strict-boolean-expressions rule.

The challenge we are facing, is how auto-fix conflicts with some other rules that we believe are very valuable:

  1. eqeqeq - We agree with the philosophy of the rule: It is considered good practice to use the type-safe equality operators === and !== instead of their regular counterparts == and !=. The reason for this is that == and != do type coercion.
  2. no-eq-null - Comparing to null without a type-checking operator (== or !=), can have unintended results as the comparison will evaluate to true when comparing to not just a null, but also an undefined value.

The fixes and suggestions (specifically the auto-fix for objects when allowNullableObject is false) conflict with the above rules.

I am sure that there is good reason and a lot of discussion behind why there was a decision to use == null as opposed to === undefined, but I thought it would be worth opening a discussion to see if we can get a consensus that it would be preferable to change the behavior to align more with the core eslint rules and the philosophy behind why it is a good idea to enable them.

Fail

let obj;
if (Math.random() > 0.5) {
  obj = {};
}

if (obj) {
  console.log('I am set!');
}

Pass

// Current behavior
let obj;
if (Math.random() > 0.5) {
  obj = {};
}

if (obj != null) {
  console.log('I am set!');
}

// Preferred behavior
let obj;
if (Math.random() > 0.5) {
  obj = {};
}

if (obj !== undefined) {
  console.log('I am set!');
}

Additional Info

@justinhelmer justinhelmer added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Dec 6, 2022
@bradzacher
Copy link
Member

align more with the core eslint rules

  • no-eq-null is a non-recommended lint rule.
  • eqeqeq is a non-recommended lint rule. Furthermore it can also be configured to explicitly enforce that you never use strict equality with null.

There is nothing to align with in ESLint core - these are just two optional, stylistic rules that can be used to enforce a certain code style.


I'm personally opposed to strict equality checks against null because from my experience 99.999999% of the time you can and should treat null and undefined as if they were the same value in conditionals.


I am sure that there is good reason and a lot of discussion behind why there was a decision to use == null as opposed to === undefined

IIRC it was because the rule itself doesn't specifically distinguish between null and undefined - it just treats the existence of one (or both) of those types as the single "nullish" case.
So when fixers were added it was very simple to write one fixer using double equals because it fit into the existing logic and available information without any refactoring or additional branching.

In hindsight, it does also allow for a simpler rule because we also didn't need to write three separate fixers (one for === null, === undefined and === null && === undefined).

Should we change it? I'm leaning towards no because:

  • there is quite a bit of extra complexity and code (I.e. Maintenance burden) required to make this change
  • it is a purely stylistic decision as to the check that's used
  • the fixers were added 18m ago (feat(eslint-plugin): [strict-bool-expr] add fixes and suggestions #2847) and there's been very little complaint - suggesting users either (a) don't care about choosing one style over the other, (b) prefer the double equals style, or (c) are okay with manually fixing to triple equals style
  • changing it to triple equals would then upset users who prefer double equals. So really it would be an option, not a full change.
  • regardless of the fix style, there are other rules which will enforce the code conforms to the codebase style
    • to quote the eslint docs on best practices for fixers: "Since all rules are run again after the initial round of fixes is applied, it’s not necessary for a rule to check whether the code style of a fix will cause errors to be reported by another rule."

Should we add an option? I'm leaning towards no again because:

  • the first point above - added complexity
  • the third point above - I don't think many people would care enough to bother to configure the fix style

Not blocking on this, but without a really killer reason or there is a lot of push from users - I don't think it's worth the maintenance burden.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Dec 6, 2022
@sindresorhus
Copy link

the fixers were added 18m ago (#2847) and there's been very little complaint - suggesting users either (a) don't care about choosing one style over the other, (b) prefer the double equals style, or (c) are okay with manually fixing to triple equals style

In my case, I didn't use the rule because it used to be buggy and too limited. And now, I cannot enable the rule as the auto-fix fixes to the wrong thing (not ===).


xojs/eslint-config-xo-typescript#71

@bradzacher
Copy link
Member

I'm okay if we add an option to control the null/undefined fixer style.

interface Options {
  nullishFixStyle?: 'double-equals' | 'triple-equals';
}

const defaults: Options = {
  nullishFixStyle: 'double-equals',
};

Where double-equals is the current behaviour which fixes to == null, and triple-equals will pick the best fix between === null, === undefined or === null && === undefined depending on the types.

@bradzacher bradzacher added accepting prs Go ahead, send a pull request that resolves this issue and removed awaiting response Issues waiting for a reply from the OP or another party labels Jan 20, 2023
@justinhelmer
Copy link
Author

Love it! Hopefully there are other willing contributors who have more time than I do right now to submit a PR for this.

I'll keep an eye on it.

@justinhelmer
Copy link
Author

(cc @sindresorhus)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

3 participants