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

Docs: [prefer-nullish-coalescing] doesn't work with strictNullChecks: false. #5748

Closed
4 tasks done
jsalvata opened this issue Oct 5, 2022 · 6 comments · Fixed by #5988 or #6174
Closed
4 tasks done

Docs: [prefer-nullish-coalescing] doesn't work with strictNullChecks: false. #5748

jsalvata opened this issue Oct 5, 2022 · 6 comments · Fixed by #5988 or #6174
Labels
accepting prs Go ahead, send a pull request that resolves this issue documentation Documentation ("docs") that needs adding/updating fix: strictNullChecks issues that were fixed by turning on strictNullChecks good first issue Good for newcomers package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@jsalvata
Copy link

jsalvata commented Oct 5, 2022

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=4.8.4&sourceType=module&code=DYUwLgBAHgXBDOYBOBLAdgcwgHwgVzQBMQAzdEQgbgCgodcByBmoA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Y6RAM0WlqYSNkAC1pkA9gEMkyMswDm6KL2jjokcGAC+ILUA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylAGYBDVCXTgwAXxCygA

Repro Code

let x: string | undefined;
x || '';

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  "rules": {
    "@typescript-eslint/prefer-nullish-coalescing": "error"
  }
};

tsconfig

{
  "compilerOptions": {
    "strictNullChecks": false
  }
}

Expected Result

An error should be reporting for the ||, which should be replaced with ??.

Actual Result

No error is reported.

Additional Info

The same rule works if strictNullChecks is changed to true.

This makes sense because strictNullChecks: false essentially erases | null and | undefined from all types.

Ideally the rule should work anyway, assuming all types to be nullable. If that's not possible, a note should be added to the documentation so people do not spend a whole afternoon trying to understand why the rule doesn't work, as I did.

Versions

package version
@typescript-eslint/eslint-plugin 5.39.0
@typescript-eslint/parser 5.39.0
TypeScript 4.4.4
ESLint 7.32.0
node 12.22.6
@jsalvata jsalvata added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Oct 5, 2022
@bradzacher
Copy link
Member

This is expected.
With the compiler option turned off TS essentially erases the nullish types from everything. So when we inspect the types - they don't include the nullish types.

Use the flag - it's a VERY bad practice to not use it.

@bradzacher bradzacher closed this as not planned Won't fix, can't repro, duplicate, stale Oct 5, 2022
@bradzacher bradzacher added working as intended Issues that are closed as they are working as intended fix: strictNullChecks issues that were fixed by turning on strictNullChecks and removed bug Something isn't working triage Waiting for maintainers to take a look labels Oct 5, 2022
@jsalvata
Copy link
Author

jsalvata commented Oct 6, 2022

Even if working as intended, which I agrree with, a note should be added to the documentation for the sake of the rule user's.

Bad practice indeed, but the flag is there for a reason: sometimes it is unavoidable, at least temporarily.

@JoshuaKGoldberg
Copy link
Member

True! Marking as accepting PRs for a docs update to mention this on https://typescript-eslint.io/rules/prefer-nullish-coalescing.

At least one other rule has a similar limitation. You can probably copy the phrasing from it/them.

@JoshuaKGoldberg JoshuaKGoldberg added documentation Documentation ("docs") that needs adding/updating accepting prs Go ahead, send a pull request that resolves this issue and removed working as intended Issues that are closed as they are working as intended labels Oct 6, 2022
@JoshuaKGoldberg JoshuaKGoldberg changed the title Bug: [prefer-nullish-coalescing] doesn't work with strictNullChecks: false. Docs: [prefer-nullish-coalescing] doesn't work with strictNullChecks: false. Oct 6, 2022
@JoshuaKGoldberg JoshuaKGoldberg added the good first issue Good for newcomers label Oct 6, 2022
@bradzacher
Copy link
Member

bradzacher commented Oct 7, 2022

We should give it the same treatment as rules like strict-boolean-expressions where the rule will explicitly error if you don't have the option turned on.

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

I would say we can do that change without a breaking change because the rule is LITERALLY useless without the option.

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Nov 2, 2022

For anybody interested in this as a good first issue: feel free to do either or both of:

  • Adding an explicit notice to the docs page (prefer-nullish-coalescing.md)
  • Adding a similar option to the rule (prefer-nullish-coalescing.ts)

We can file a followup issue if you only tackle one and not the other. 🙂

@JoshuaKGoldberg
Copy link
Member

Reopening so @cparros can tackle the second part 😄

@typescript-eslint typescript-eslint unlocked this conversation Nov 30, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue documentation Documentation ("docs") that needs adding/updating fix: strictNullChecks issues that were fixed by turning on strictNullChecks good first issue Good for newcomers package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
3 participants