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

prefer-nullish-coalescing rule #14037

Closed
koooge opened this issue Jan 26, 2021 · 8 comments
Closed

prefer-nullish-coalescing rule #14037

koooge opened this issue Jan 26, 2021 · 8 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects

Comments

@koooge
Copy link

koooge commented Jan 26, 2021

Hi there,

Please describe what the rule should do:
Almost same as typescript-eslint's prefer-nullish-coalescing.

https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md

I think that could be included in the eslint feature. It seems to depend on the type of information of typescript though.

What new ECMAScript feature does this rule relate to?

https://github.com/tc39/proposal-nullish-coalescing

What category of rule is this? (place an "X" next to just one item)

[ ] Warns about a potential error (problem)
[X] Suggests an alternate way of doing something (suggestion)
[ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about:

a || b; // warn
a ?? b;

if (a || b) {} // warn if ignoreConditionalTests = false
if (a ?? b) {}

(a && b) || c; // warn if ignoreMixedLogicalExpressions = false
(a && b) ?? c; 

Why should this rule be included in ESLint (instead of a plugin)?
Since Nullish Coalescing operator became a feature of EcmaScript not only typescript.

Are you willing to submit a pull request to implement this rule?
Not for now.

@koooge koooge added feature This change adds a new feature to ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Jan 26, 2021
@mdjermanovic mdjermanovic added the needs info Not enough information has been provided to triage this issue label Jan 26, 2021
@eslint-deprecated
Copy link

Hi @koooge, thanks for the issue. It looks like there's not enough information for us to know how to help you.

If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

Requesting a rule change? Please see Proposing a Rule Change for instructions.

If it's something else, please just provide as much additional information as possible. Thanks!

@mdjermanovic
Copy link
Member

Hi @koooge! Can you please update the proposal with the code examples?

@mdjermanovic mdjermanovic added this to Triaging in Triage Jan 26, 2021
@koooge
Copy link
Author

koooge commented Jan 26, 2021

Hi @mdebruijne, I wrote simple code examples. I realized there is a bit different from typescript-eslint's. It seems not simply warn ||, using the type of information. Thank you.

@mdjermanovic mdjermanovic added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed needs info Not enough information has been provided to triage this issue triage An ESLint team member will look at this issue soon labels Jan 26, 2021
@mdjermanovic
Copy link
Member

Thanks for the update!

a || b; // warn
a ?? b;

So the rule would basically disallow || in favor of ??? I'm not sure if it makes sense without type information, the rule would be probably unusable due to a lot of false positives.

@mdjermanovic mdjermanovic moved this from Triaging to Evaluating in Triage Jan 26, 2021
@binury
Copy link
Contributor

binury commented Feb 22, 2021

At least in my experience, most of the time a short-circuit evaluation works out as it was intended when written.

One downside of preferring coalescence over short-circuits is that usages where ?? is required no longer stand out. For example:

this.precision = config.precision ?? 4

Here we can infer by the necessity of nullish coalescing in the expression that 0 is also an acceptable value of config.precision

const canViewContent = user.age > 21 ?? user.isExempt

Whereas here we should ultimately ignore the usage of ?? and mentally equate it a short-circuit eval

this.precision = config.precision ?? 4
const canViewContent = user.age > 21 ?? user.isExempt

How about now?

I understand the inclination to treat ?? like a better strict-mode version of || but it's simply different

—Sorry I had more to say but my battery is dying; gotta run

@mdjermanovic
Copy link
Member

There are probably many places in a codebase where || can (and should) be replaced with ??, but sometimes || has to be || and I don't think we can reliably detect that.

@btmills
Copy link
Member

btmills commented Mar 18, 2021

I agree that there's too much risk of false positives here. I'd lean toward leaving it in @typescript-eslint because that plugin rule can tell more accurately when one or the other is needed.

@nzakas
Copy link
Member

nzakas commented Aug 28, 2021

We won’t be moving forward with this, so closing.

@nzakas nzakas closed this as completed Aug 28, 2021
Triage automation moved this from Evaluating to Complete Aug 28, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 25, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

No branches or pull requests

5 participants