feat(eslint-plugin): [prefer-nullish-coalescing] ignoreStrings option

[Crow-EH]

Hi everyone ! 👋

It's my first contribution to this project, so don't hesitate to correct me if anything is wrong.
Also, I didn't find any issue about this, so I directly created a PR. Once again, just tell me if I'm doing it wrong. 👌

Original use case

TL;DR: I need || for strings, but please tell me if there's a better solution for the community

I work on a pretty big "middleware" kind of API, of which one of the main job is to clean the data from a multitude of other services, before sending it to many kind of front-end clients on different platforms.

One of its cleaning tasks is to replace any empty string with either undefined or a fallback value.

We really want to use prefer-nullish-coalescing to enforce the use of ??, to avoid accidentally removing false or 0, but we also don't want to replace aString || 'fallback' by !aString ? 'fallback' : aString. Keep in mind that most of the time it even looks like a.path.that.i.cant.always?.trust || 'fallback' VS !a.path.that.i.cant.always?.trust ? 'fallback' : a.path.that.i.cant.always.trust.

This problem might be a bit specific and I'm 100% open to code a better solution, answering everyone needs, if required.

Solution:ignoreStrings (just copying the doc I just commited)

Setting this option to true will cause the rule to be ignored when the left-hand's TypeScript type is string, whether it may be nullish or not.

This can be useful if you want to enforce the use of ?? on every types but string, because you want to handle empty strings.

Incorrect code for ignoreStrings: false, and correct code for ignoreStrings: true:

declare const nullString: string | null;
const emptyString = '';
const a = nullString || 'fallback';
const b = emptyString || 'fallback';
// a === 'fallback'
// b === 'fallback'

Correct code for ignoreStrings: false:

declare const nullString: string | null;
const emptyString = '';
const a = nullString ?? 'fallback';
const b = emptyString ?? 'fallback';
const c = !emptyString ? 'fallback' : emptyString;
// a === 'fallback'
// b === ''
// c === 'fallback'

Please tell me if there's any error, subpar code or any rephrasing needed.

Thank you very much !

[typescript-eslint]

Thanks for the PR, @Crow-EH!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[codecov]

Codecov Report

Merging #1569 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1569      +/-   ##
==========================================
+ Coverage   95.56%   95.56%   +<.01%     
==========================================
  Files         142      142              
  Lines        6519     6523       +4     
  Branches     1855     1856       +1     
==========================================
+ Hits         6230     6234       +4     
  Misses        105      105              
  Partials      184      184

Impacted Files	Coverage Δ
...lint-plugin/src/rules/prefer-nullish-coalescing.ts	98.21% <100%> (+0.13%)	⬆️

[Crow-EH]

I made an error in the doc. Will fix it asap.

[Crow-EH]

Done

[Retsam]

Not a maintainer, but my $0.02:

It seems the main benefit of ?? is that it doesn't make exceptions for the falsy values for primitive types, it handles all values consistently. So an option for disabling that behavior seems odd to me. (If you're dealing with non-primitive values, there's no difference in behavior between ?? and ||)

While certainly there are places where you do want to handle the falsy and the nullish case together, I can't think of any reason why a codebase would always want to do so. And, for those exceptions, I actually think it's better to override the rule with a //eslint-disable comment than to configure the rule to not error on these cases, because that makes it clear that the handling of falsy values is intentional.

So, in your specific case, I actually think this is good: I can tell that the intended behavior is to replace '' with 'fallback'.

// eslint-disable-line @typescript-eslint/prefer-nullish-coalescing
a.path.that.i.cant.always?.trust || 'fallback'

Or to be even more explicit:

//
function replaceEmptyStringOrNullish(val: string | null | undefined, defaultVal: string) {
    return val ? val : defaultValue;
}

replaceEmptyStringOrNullish(a.path.that.i.cant.always?.trust, 'fallback');

So I'd suggest that typescript-eslint should resist adding configuration for this specific case.

[bradzacher]

I would tend to agree with @Retsam here.
I think your intentions are ace for your specific use case, but they do not make for a good option on a lint rule.

For rules I look to the intention of the rule when adding options, to see if the option is working toward making the rule more flexible whilst still achieving its goal.

In this case, the intention of prefer-nullish-coalescing is to ensure you use nullish coalescing instead of logical ors so that your code base explicitly and clearly handles both nullish and falsey values.
(I see it as a sister rule to strict-boolean-expressions, which has a similar goal, but for boolean language constructs.)

The problem with this option is that it's too far reaching - it disables it for every single nullish string; it lacks the contextual information you've provided in the post.
It cannot differentiate between a string which should not be allowed to be an empty string, and a string which can be an empty string; which means this option creates a large hole in your codebase.

---

IMO a better solution to this would be something similar to what Retsam mentioned - a utility function, though I would take it one step further.
There is a great trick you can do in TypeScript with primitive types which is generally called "branding". It's best explained through an example:

type NotEmptyString = string & { __notEmptyBrand: unknown };

/**
 * Ensures that a string is not null or undefined, and
 */
function notEmptyString(value: string | null | undefined, fallback: string): NotEmptyString {
  if (!fallback) { throw new Error('fallback must be a non-empty string'); }
  return (value ? value : fallback) as NotEmptyString;
}

const one = notEmptyString('', 'fallback'); // typeof one === NotEmptyString
const two = notEmptyString('foo', 'fallback'); // typeof two === NotEmptyString

const three = notEmptyString(notEmptyString('foo', 'fallback'), 'fallback'); // note that you can assign NotEmptyString to anywhere that accepts string

declare function onlyAcceptsNotEmptyString(value: NotEmptyString): void;

onlyAcceptsNotEmptyString(one); // no error

declare const regularString: string;
onlyAcceptsNotEmptyString(regularString); // ERROR because string is not assignable to NotEmptyString

I use this technique in our codebase to ensure that I only pass canonical paths around. The typescript codebase also uses this technique for a similar use case (which is where I got the idea from).

This is an awesome way to do some form of validation on a primitive value, and represent it in your types, which means that your validation is backed by the typescript compiler - removing all ambiguity, and cognitive load.
It's also great because at any given point in your code, you can very easily tell if a given string has been "sanitised" by just inspecting its type in your IDE - it it typed as string, then it's not sanitised, if it's NotEmptyString then it is sanitised.

[Crow-EH]

Hi @Retsam, @bradzacher, and thank you for your inputs !

@Retsam I agree with you that typescript-eslint should not become an impossible mess of inter-breaking configuration flags, and I also know we would only realize when it's too late, so the maintainers have to carefully pick which conf makes the cut.

I honestly didn't know whether this flag was hurtful or not for the community, and my intention was always to find out here.

@bradzacher I understand your point clearly, and your thought process is crystal clear. I didn't think of this rule like that (the "prefer" might have fooled me 😅).

Your way of dealing with it is actually pretty clean. It might be a bit more verbose, but the pros overweight the cons by a lot IMO. I'll ask my teammates what they think about it.
Thanks for the tip !

I'm OK with both of you, please close this PR whenever.

PS: FYI @Retsam, we actually (try to) remove all the empty strings before sending any response back: All of the front applications are heavily "server-driven" and we wouldn't want any empty divs showing up (turns out '' == undefined is not true everywhere). ATM we use || for this on about 1k lines in a small sized codebase (~11k statements according to nyc). That's actually a lot of work and I surely would never want to add 1k // eslint-disable-line. 😱
(Not our data by the way, we pull it from a dozen of third parties and aggregate it)

EDIT: Actually, I'll close it. Thank you for your time :D