Skip to content

[prefer-nullish-coalescing] add option to only flag where behavior would be the same #1265

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.

Sign up for GitHub

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

Jump to bottom
Closed
felixfbecker opened this issue Nov 26, 2019 · 2 comments · Fixed by #1272
Closed

[prefer-nullish-coalescing] add option to only flag where behavior would be the same #1265

felixfbecker opened this issue Nov 26, 2019 · 2 comments · Fixed by #1272
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@felixfbecker
Copy link

Repro

{
  "rules": {
    "@typescript-eslint/prefer-nullish-coalescing": "error"
  }
}
function fn(bar?: string) {
	const foo = bar || 'defaultval' // not an error
}

interface SomeObject {
	someProp: boolean
}
function fn2(bar?: SomeObject) {
	const foo = bar || { someProp: true } // error
}

Expected Result

This should not trigger a rule error, because using ?? would be a different behavior: It would not apply the default value if bar is an empty string.

I would expect there to be an option so that it only flags cases where it can confidently say that there is no change in behavior, like when the type is an object. Maybe something like ignoreTypes: ["number", "string", "boolean"].

Actual Result

It flags uses of || where the behavior is different than ??, i.e. with strings, booleans and numbers.

Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 2.9.0
@typescript-eslint/parser 2.9.0
TypeScript 3.7.2
ESLint 6.6.0
node 12.11.0
npm 6.13.1
@felixfbecker felixfbecker added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Nov 26, 2019
@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Nov 26, 2019
@bradzacher
Copy link
Member

bradzacher commented Nov 26, 2019 •
edited
Loading

I thought about this, but the problem is that this rule would pretty much never fire if it were to be disabled for falsey primitives. Taking string, number, boolean, and bigint out of the equation leaves you with just object and symbol.

From my survey of the usage of nullish coalescing on a large codebase (FB's codebase): it's rare that I see people use chained logical operators for things other than the falsey primitives and arrays. I think the reason for this is that it's rare that you've got an object that is easy to construct with defaults, but it's easy to default a falsey primitive. Also generally nullish objects are handled instead via optional chaining, and people default the nullish output using nullish coalescing (i.e. still operating on falsey primitives).

Could potentially convert the fixer to a suggestion in these cases, so it is not automatically applied.

As an aside (this is a personal preferece) - you should endeavour to not rely upon the falsey behaviour of the primitives. Being explicit and showing clear intention for both null and falsey values is much clearer, and leads to a safer codebase.

This is why flow has a compiler error built into strict mode, which prevents you from using the falsey behaviour (and we have the strict-boolean-expressions rule which does a similar thing).

@felixfbecker
Copy link
Author

As an aside (this is a personal preferece) - you should endeavour to not rely upon the falsey behaviour of the primitives. Being explicit and showing clear intention for both null and falsey values is much clearer, and leads to a safer codebase.

I agree - the problem is that adding this rule to a codebase, there are countless violations, and changing them is dangerous as you cannot prove that no part relies on this behavior. So I'd rather have at least a subset checked at first and slowly transition to using it for everything.

Also, I think in any case where the input comes from outside your system, e.g. an API request, you usually do want to treat an empty string as if a string was not provided (undefined) and doing so leads to a safer codebase. I agree for anything within the system boundaries though.

Could potentially convert the fixer to a suggestion in these cases, so it is not automatically applied.

I think it should definitely be a suggestion as it changes runtime behavior.

@bradzacher bradzacher added enhancement: plugin rule option New rule option for an existing eslint-plugin rule has pr there is a PR raised to close this and removed awaiting response Issues waiting for a reply from the OP or another party labels Nov 27, 2019
@bradzacher bradzacher mentioned this issue Nov 27, 2019
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(eslint-plugin): [prefer-null-coal] opt for suggestion fixer typescript-eslint/typescript-eslint
2 participants
@bradzacher @felixfbecker