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? 😄

[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.

[phaux]

To enforce ternary in JSX you can use no-restricted-syntax like this:

{
  "no-restricted-syntax": [
    "error",
    {
      "selector": "JSXElement > JSXExpressionContainer > LogicalExpression[operator!='??']",
      "message": "Please use ternary operator instead"
    }
  ]
}

[marneborn]

Just adding to @phaux's suggestion above:

{
  "no-restricted-syntax": [
    "error",
    {
      "selector": "JSXElement,JSXFragment > JSXExpressionContainer > LogicalExpression[operator!='??']",
      "message": "Please use ternary operator instead"
    }
  ]
}

[bobaaaaa]

@marneborn your example does not work. Try this:

{
  "no-restricted-syntax": [
    "error",
    {
      "selector": ":matches(JSXElement, JSXFragment) > JSXExpressionContainer > LogicalExpression[operator!='??']",
      "message": "Please use ternary operator instead"
    }
  ]
}

[alesmit]

It would still be great to have a rule to detect non-boolean type for inline rendering, without needing to enforce ternary expressions. I thought the goal was to spot code like this:

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

rather than banning any logical expression with && in JSX. Is there any way to achieve this with custom rules?

[krawaller]

@alesmit I have an implementation of exactly that in #2077 (which was shot down)

[MichaelDeBoey]

TS can’t match “integer” or “number greater than 3”

@ljharb I don't think the new rule should do this.
It can just always use the ternary instead of && operator when the rule is enabled, as that way it's always safe.
If people don't want that, they don't need to enable it 🤷‍♂️

[ljharb]

@MichaelDeBoey right. and i'm saying that that would be very harmful, because quite often, && and || are the best operator to use. Eliminating them entirely just because of this very niche edge case is not beneficial.

[rijk]

TS can’t match “integer” or “number greater than 3” or “a string that matches a regex”,

How does that matter here? The discussion has somewhat derailed in the past days, as said the TS rule works perfectly and not by "banning" && or ||, but by simply adding !! in front of non-boolean variables used in logical AND operator inside of JSX.

[ljharb]

@rijk a component that takes numbers > 0, for example, is perfectly safe to use && and ||.

[MichaelDeBoey]

@ljharb It is indeed safe to use, but that's not the point of this request imo.
The point is to add a rule to always use ternaries instead of && and ||, even though it would be perfectly safe to use them in certain cases.

[ljharb]

I understand that. I'm saying that such a rule would be actively harmful since it would be penalizing the common case for the sake of an edge case.

[MichaelDeBoey]

You call it "penalizing the common case", but some people will see it as "consistent behavior", as that way they don't need to think about the fact if it safe or not.

Having this rule, but disabling it by default is a good idea imo, as that will leave the people the active/determined choice if they want this consistency or not.

[ljharb]

You can already opt into it with no-restricted-syntax. I'd still rather have a rule that knew about React types/propTypes, and was able to penalize a much smaller set of otherwise-valid use cases.

[kachkaev]

penalizing the common case for the sake of an edge case

Yep, that’s what what good coding styles do! For example, a lot of folks use eqeqeq to write myVar === "value" instead of myVar == "value". Why, if the latter is one character shorter (so better in all respects)? Well, because we don’t what to write myVar == "" one day and then debug false == "" for a few hours.

The same principle applies here. If condition ? <Jsx /> : undefined (or null) works for any condition, there is no point alternating between && and ? :. Doing so will just drag more of the team’s time on PR reviews or bug hunting.

[ljharb]

@kachkaev and if this were general javascript, the principle would apply. This is React - where we basically always have type information about the props - so we can accurately warn about many of the cases.

[kachkaev]

I don’t understand what you mean, sorry. How would ‘information about props’ work here?

const MyComponent = () => {
  const something = useSomething();
  
  return <div>{something && <Something />}</div>;
}

Remember, that TypeScript is out of scope – that’s separate subject: #2073 (comment).

The advantage of a new JS-only rule over no-restricted-syntax is obvious. It is accessibility (and therefore adoption). If we have no-restricted-syntax in company rules and then want to restrict more unrelated cases in a specific repo, we need to compose no-restricted-syntax and that’s tedious. I do that, but I wish I did not have to.

[ljharb]

Yes, in that specific case, we'd have no idea what kind of thing it is - and I think the best choice is to not warn on it, or at least, to offer a suggestion but not a warning.

I think it would be very reasonable to have a rule that, when it knew the type with certainty, provided a warning or was silent, but when it didn't, only provided a suggestion.

[MichaelDeBoey]

If the rule has at least the option to always report, I wouldn't be against it.

But as @kachkaev already mentioned with the eqeqeq example: for me it's all about consistency and not having to think about if it could be safe to use &&/|| or not
I just want a rule that always prevents using them, so I'm certain that 100% of the time it's safe.

[rijk]

Oh, I missed that that was what you were arguing. In that case, I agree with @ljharb ; why force people to use ternaries if there is another way (using !!)?

I personally like having the && option and I think it is more readable in some cases, like an if statement without an else;

{editable && <EditButton />}

I find that definitely easier to read and parse than:

{editable ? <EditButton /> : undefined}

Especially when you have a longer piece of code;

{editable ? (
  <div>
    <EditButton 
      onClick={() = {
        // ...
      }}
    /> 
  </div>
) : undefined}

you have to scan all the way to the bottom to find out that it is actually just an if, and not an if/else. To me that adds unnecessary cognitive load, that is why I don't like it. I also have a more philosophical objection to disallowing people to use a core part of the language, just because it could backfire if used wrong. That should only be reserved for the most extreme cases (e.g. eval).

[MichaelDeBoey]

why force people to use ternaries if there is another way (using !!)

I also have a more philosophical objection to disallowing people to use a core part of the language, just because it could backfire if used wrong.

I don't want to force people automatically, I want to make it an auto-fixable option.
That way this rule can be enabled and developers don't have to think about it anymore (just like we have with Prettier).
And the (possible) bug this prevents will never happen again.
I only see benefits here.

I'm also more in favor of using Boolean instead of !! btw, but that's another discussion 😛🙈

you have to scan all the way to the bottom to find out that it is actually just an if, and not an if/else.

You can always invert the condition so you'll have

{condition ? null : <Component />}

Although I wouldn't do that in your given example as I like to keep my conditions as positive as possible (this is a bit smaller cognitive load).

[golopot]

This is handled by #3203

[MichaelDeBoey]

I can confirm, #3203 fixed the use-case I was pointing out above & in #3153

Huge thanks @Belco90!

[pke]

Thanks a lot! Tried it and the ternary fixing option works great. The coerce however has problems with multiple conditions like this:
cond1 && cond2 && <Render .../>

which is autofixed to

!!!!!!!!!!!!!!!!!!!!!cond1 && cond2 && <Render .../>

[ljharb]

@pke i believe we have an open issue for that; if not, please file one.

Fixed by #3203.

[pke]

Ah ok... will file one if none exist later.
Also the rule detects !!cond1 && !!cond && <Render.../> as invalid. Should it?

[ljharb]

Nope, that should be fine.

[pke]

Since there is no testcase for multiple conditions I guess this a bug then?

[Belco90]

Ah ok... will file one if none exist later.

Also the rule detects !!cond1 && !!cond && <Render.../> as invalid. Should it?

This was already fixed in #3299 but hasn't been released yet. You can see in that PR some new tests to make sure several boolean chained are correct.

[adrienjoly]

As suggested in typescript-eslint/typescript-eslint#2770 (comment), we ended up adding this ESLint plugin: eslint-plugin-jsx-expressions - npm, to detect & prevent Unexpected text node: . A text node cannot be a child of a <View>. errors when developing conditional JSX/TSX code.