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]

@rijk TS can’t match “integer” or “number greater than 3” or “a string that matches a regex”, to list some examples. PropTypes can do whatever JS can; TS can only do what TS can, which very much does not include all JS semantics.

[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: https://github.com/yannickcr/eslint-plugin-react/issues/2073#issuecomment-945025341.

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 https://github.com/jsx-eslint/eslint-plugin-react/pull/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 https://github.com/jsx-eslint/eslint-plugin-react/pull/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 https://github.com/typescript-eslint/typescript-eslint/issues/2770#issuecomment-945024455, 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.