Eslint-plugin-react: Feature: Disallow non-boolean type for inline If with logical && operator

[cburgmer]

We are making use of inline conditional guard statements a lot like:

    <div>
      {unreadMessages.length > 0 && <h2>Hey</h2>}
    </div>

We often run into bugs where a string literal 0 is rendered, because a short hand notion was used, which react does not allow:

    <div>
      {unreadMessages.length && <h2>Hey</h2>}
    </div>

React will not render a boolean value, while it does for integer values like 0 and 1.

If possible, I'd like to detect this pattern and flag it early. Apologies if this already exists, I checked for "conditional" and "inline" and couldn't find a match.

[ljharb]

This is a tricky one, because if it’s any falsy value besides a number, it will work properly.

Once the nullish coalescing operator lands, and that’s a better alternative, it might make more sense to have a rule - right now, such a rule would probably be more noisy than helpful, I’m afraid.

[krawaller]

I managed to miss this issue, and implemented a rule as suggested here by @cburgmer in PR #2077.

It protects solely against using the truthiness of a length prop as guard, which as @ljharb said in the PR might be too narrow to be useful.

I don’t see an immediately obvious way of expanding the rule to cover more numeric guard cases, but my gut feeling is that the .length case is common enough to justify the rule.

Then again, it might be that I want to believe that just so I’m not alone in being a schmuck (did exactly this mistake in production code twice)...

[ljharb]

Relying on the implicit truthiness of .length anywhere imo is a bad idea (for this reason as well as many others) - if you always do a strict number comparison with .length, you won't run into the problem there at all (which the airbnb styleguide requires, ftr).

[krawaller]

Hmm. Interesting. I was focused on the fact that implicit length truthiness is especially dangerous in JSX, earning me the title ”0 of the week” on the main company whiteboard.

But I see your point - you’ll live a happier life if you _never_ rely on it, jsx or not.

Still, in most other environments it’s more arrogant than unsafe to do it, no? There I’d classify this rule as ”coding style”, while in JSX it’s a ”potential error” (actually pretty much a guaranteed one).

@ljharb, would you consider this rule if we add a ”forbidEverywhere” option, disallowing boolean casting of dot length anywhere as per the airbnb styleguide?

[krawaller]

Another way of making the rule more useful could be to allow the user to add props other than .length to also be included in the check. Maybe I'm using lots of Set:s in my codebase and could thus add .size, for example.

[ljharb]

I don't think it's really something a linter can do - there's no guarantee a length property is even a number (not that I can think of a use case for that). I think at some point the solution is always going to boil down to actual tests.

[krawaller]

A linter would've saved me, twice! But then again, so would writing better code. :)

I agree there's no guarantee, but I think the argument is that there doesn't need to be, and that erroneous casting of .length is common enough to warrant a rule.

Also, protecting against this particular mistake with tests seems a bit off - would you really write a test to see that you didn't render a zero?

But I'm liking the idea of enforcing the AirBnB rule of never casting length to boolean, and so we'll probably widen the rule to do that and deploy it to our linting suite outside of the React package.

Thank you for the input!

[cburgmer]

I might have misunderstood, the only linting rule I've sound so far is https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/docs/rules/explicit-length-check.md, which is not included in the Airbnb rule set.

To reproduce:

$ npm install eslint-config-airbnb-base
$ npx install-peerdeps --dev eslint-config-airbnb-base
$ cat > h.js << EOF
heredoc> const list = [];
if (list.length) {
  list.pop();
}
heredoc> EOF
$ npx eslint .
$

[krawaller]

@ljharb: I want to make a last attempt at convincing you!

In the 2.5 months passed I've seen the arr.length && <Something/> mistake thrice more in the wild. Yes, a rule that could catch all numeric guards would be better, and a rule that could catch all non-boolean guards would be better still. But I think the flawed arr.length usage is common enough to warrant a dedicated rule. It might not be a broad rule, but I argue it'll save more than enough grief to pay for the inclusion.

To me the bottom line is this: we have a very common pattern in normal JS that might be frowned upon by some, but it is still widely used and perfectly safe. In JSX it suddenly isn't safe anymore.

So yes, you could argue that users should use a general explicit-length-check rule like the one linked by @cburgmer above. But to me that rule enforces an opinion, which falls in a different category. The proposed jsx rule catches the usage in JSX specifically, which is always a mistake.

[krawaller]

Half a year later, new assignment, ran this rule on the (large) codebase. Found 10 violations, all of which were real dangers of rendering a 0 to screen. Codebase is otherwise in very good shape with sound linting, and the devs are experienced and driven.

I'm even more convinced than before that this rule deserves a place in a React-dedicated rules package.

[MuYunyun]

I managed to miss this issue, and implemented a rule as suggested here by @cburgmer in PR #2077.

By the way, how to config make it take effect? :smile:

[marhaupe]

I'd like a rule for that as well. I'm not sure if this package treats react-native as first class citizen, but if it does, another point to consider is that a lint rule like this would actually prevent errors in react-native. Unlike in the browser, you can't render text outside of <Text/> components. If you do, your app crashes. Another source of errors are cases like this:

{  
  props.text && <Text>{props.text}</Text>
}

This would just render an empty string and just won't affect a react app. In react native however, we'd be trying to render a string outside of a Text component too.

[pke]

Half a year later, new assignment, ran this rule on the (large) codebase.

Where is the rule? You refer to the length one?

Will a more broader rule be considered? Especially in the react-native env such rule would help to prevent serious bugs in the code base. Maybe it could only be implemented as a typescript rule, because the TS compiler knows about types (and RN projects are usually type script projects).

[cburgmer]

I've seen eslint rules in conjunction with TypeScript. Maybe the rule I'm looking will benefit from the type information. Would such a rule still be welcome here?

[ljharb]

@cburgmer as long as it's still useful with no type information, absolutely.

[phaux]

One solution is to use "@typescript-eslint/strict-boolean-expressions": ["error", { "allowNumber": false }].

Alternative solution which doesn't require types would be to create a rule which disallows a && b in JSX altogether and always autofixes it to a ? b : null

[ljharb]

that wouldn’t be appropriate tho when a is a boolean type :-/

[phaux]

Why though?

I often use ternary in JSX even when the else branch is just null. I like it because it's consistent. It also kinda forces you to at least consider displaying some message like "No data to show" which is usually a nice UX. I once had to refactor a bunch of components to show these empty states in UI and if I used ternaries from the beginning it would be much easier. It would be useful rule for enforcing style, and if you want actual bug-catching rule then there's strict-boolean-expressions.

[ljharb]

"a nice UX" depends very much on the context.

[cburgmer]

a ? b : null

Just to share a painful learning from today: The null here is rather important, and "" should be avoided, although it looks the same to the user. What happens is that for "" React will render an empty text node, which in our case broke our xpath query we are using for our browser-based tests. //span[contains(text(), "my caption"] will not behave as expected if an empty text node is created alongside the caption:

The following JSX

<span>
  {false ? "whatever : ""}
  "my caption"
<span>

will result in this HTML (quotes used to highlight the text nodes)

<span>
  ""
  "my caption"
</span>

[ljharb]

@cburgmer to be fair tho, relying on XPath queries like that is inherently brittle, so the flaw there isn't use of the empty string.

[nathggns]

Would very much like a rule that enforces ternary usage over && for conditional rendering in JSX.