Proposal: tsc backed jsx conditional rendering rule

[pke]

Over at jsx-eslint/eslint-plugin-react#2073 we are discussing a rule that only the TSC could help with.
It would require to detect if a conditional rendering check for react JSX blocks of code is a true boolean value.

Do you think the JSX world and this eslint plugin could somehow be combined to create a rule (in a new package?) for this particular bad coding error (that will crash any react-native app)?

[bradzacher]

It seems like you just want strict-boolean-expressions to also check JSX expressions as if they were logical conditions?

[pke]

I think that would do it, yes.

[bradzacher]

happy to accept PR for a new option, say checkJsxLogicals which applies the rule's logic to a JSX expression (if it is a logical expression).

[pke]

I have written eslint rules before so I am somewhat familiar with the concept. Is there any JSX support yet in this plugin I could use as a starting point? I see there is already code in this plugin that deals with JSX. Could you point me to a rule/code fragment that could get me started to add this option?

[marhaupe]

I'm just chiming in to say that I'd be happy to test this once you've implemented this option.

[bradzacher]

Best option would be to play around in https://astexplorer.net/ to get a feel for the AST you're looking to target.

From there you can craft an ESLint selector as best you can to target that AST.

Then you need to work it into the rule.

If you want a live playground for inspecting the type information that TS provides - https://ts-ast-viewer.com/ is an amazing site.

[phaux]

strict-boolean-expressions already does that and I use it for this exact purpose. You just have to set allowNumber to false (it's true by default) and you get:

<div>
  {items.length && (
    // ^ Unexpected number value in conditional. An explicit zero/NaN check is required.
    <ul>
      {items.map(item => <li>{item}</li>)}
    </ul>
  )}
</div>

The only possible downside is that if you disable allowNumber then it's disallowed everywhere, not just in JSX, so you have to write e.g. if (items.length > 0) instead of just if (items.length) (which is fine for me).

[bradzacher]

that is true - it will "just work" with the right options.
I wonder if this is just a documentation thing, then?

We can add another section to the docs that say "if you want to strictly validate your JSX logical expressions do not output values, you should use this config"

[phaux]

@bradzacher Yep. Let's add a paragraph to allowNumber and allowString options in docs that says it's useful to set them to false in react projects. I can send a PR.

It's not a problem specific to JSX though. It applies everywhere where any value can be passed.
E.g. console.log(items.length && `There are ${items.length} items.`) would log the message about length or just "0".

[phaux]

This all comes down to how logical operators are used and how you sometimes use them only for their short-circuiting behavior.
Let's say you disable allowNullableObject and then suddenly you can't do const item = itemMap.get("id") || defaultItem (yes, you simply use nullish coalescing now and it works but it's just an example).

I remember that I have already suggested to have all the allow* options accept 3 options. E.g:

• true or "always" – as it is now
• "inExpression" or something like that – only when using logical operator for it's side effect. (can be checked if AST parent is not if/for/while etc)
• false or "never" – same as now

but is it worth it to maintain such a complicated rule? I don't know.

Another idea I had in mind would be to forbid a && b and a || b altogether when they're not in condition, but that would fit better as a rule in the original eslint project.

These are just my random thoughts. I'm not proposing any of these.

[hluisson]

My team ran into pitfalls with non-boolean values while conditionally rendering in React Native, where it can cause the Invariant Violation: Text strings must be rendered within a <Text> component errors.

I looked into strict-boolean-expressions but it is pretty heavy-handed for what we are trying to achieve, especially because it applies to the logical negation operator (we deliberately use !! in several places). It seemed like it would require us to do things like {someStr !== "" && !someStr && <Text>{someStr}</Text>}

Requiring ternary expressions in JSX is an option but again feels like overkill when we're setting the right hand side of the ternary to null everywhere because it's expected that data might or might not be present.

Conditional rendering using && is a pretty normal pattern in React. What I'd really like is more specific rule for JSX expressions that will disallow:
{someStr && <Text>{someStr}</Text>
but allow (and suggest):
{!!someStr && <Text>{someStr}</Text>

This type of rule seems very achievable with typescript-eslint. I'd be happy to open to a pull request with a proposal - something like strict-jsx-conditionals or strict-jsx-expressions. However, I noticed there aren't any jsx-only rules in typescript-eslint. Would this type of rule be viewed as overly-specific?

[bradzacher]

To be clear - if we want to make a small enhancement to strict-boolean-expressions to do an additional check for JSXExpressions - then that change lives here in this project.

If the goal is something larger than that around JSX semantics / react logic - then that no longer lives within this project, and should instead be housed within eslint-plugin-react.

As a rule-of-thumb - we want to keep this project generic and framework agnostic

Type-aware lint rules aren't restricted to just this project - anyone and everyone is allowed (and encouraged) to build on top of our tooling to create type-aware lint rules and enrich the entire community.

[hluisson]

cool, I took a stab at implementing the rule using @typescript-eslint/experimental-utils and added it to our project -
eslint-plugin-jsx-expressions
just in case anyone else discovers this thread and finds it useful!

[ypresto]

@hpersson Your work should be useful! 👍

This issue was marked as accepting PRs. Could you send PR to include your rule in this repo?

[phaux]

How about a rule that is like restrict-template-expressions but it checks JSX instead of template strings:

return (
  <p>Hello {name}</p> // report if name is not string
)
return (
  <p>Items: {items.length && items.join(", ")}</p> // error: type number is not allowed in JSX expression
)

It would also be useful to not forget to format numbers using user's language:

// bad:
function ScoreCounter(props) {
  return (
    <p>{props.score} 🪙</p>
  )
}

// good:
import { FormattedNumber } from "react-intl"
function ScoreCounter(props) {
  return (
    <p><FormattedNumber value={props.score} /> 🪙</p>
  )
}

I can send PR.

[pedrodurek]

In my view, for JSX conditional rule, we don't need any configurable options, it should only accept booleans and nulls, excluding any other falsy and truthy values. That way we can tackle all edge cases for React and React Native apps.