Lint against {condition && <Element/>} in JSX

[wholesomedev]

As we discussed earlier, using {condition && <Element/>} in JSX could lead to a bug where 0 is outputted to the HTML. I can think of two solutions to this issue:

1. Lint against {condition && <Element/>} and force ternary or another solution in its place
2. Completely ban value coercion in the codebase including in places such as if (condition) and if (!condition). See this twitter thread by Sebastian McKenzie.

There might be other solutions, but I can't think of any at the moment.

[forabi]

I'm looking for a linter rule that can help with this.

• strict-boolean-expression (works for things like if (condition) but not for condition && <Element/>.
• no-implicit-coercion probably does not provide any value over what TS offers, I'll try it.

[wholesomedev]

@forabi So TS Lint rules don't apply to TSX? If they do, strict-boolean-expression should prevent condition && <Element/>.

[forabi]

@wholesomedev I think if (condition) is different from condition && <Element /> in that the former is expected to evaluate to a boolean value, while the latter is not. The rule strict-boolean-expression probably only considers boolean expressions (as the name implies)

[forabi]

I tried the different options for strict-boolean-expressions and it looks like it does what we want:

{date && (
  <time dateTime={date.toISOString()}>
    {' '}
    and was last updated on {formatDate(date, 'MMMM D, YYYY')}
  </time>
)}

src/app/components/EditorialSummary/EditorialSummary.tsx[214, 22]: This type is not allowed in the operand for the '&&' operator because it is always truthy. Allowed types are boolean, null-union, undefined-union, string, or number.

But I don't think we should use it, because it's only accidentally preventing this pattern 😄

The purpose of this rule is to prevent unnecessary fallback values for places where a boolean is expected. Things like boolean || undefined, boolean || '' are unnecessary because if boolean is true, undefined will never be used, and if boolean is false, then that's what we want (the rule assumes that the desired result is that expression always evaluates to boolean).

It prevents some patterns that I don't see any issue with:

const onChange = this.props.onChange || this.defaultOnChange; // I want the return type to always be a `function`, the rule expects a `boolean`.

ERROR: src/app/components/Collapsable/Collapsable.tsx[48, 45]: This type is not allowedin the operand for the '||' operator because it is always truthy. Allowed types are boolean, null-union, undefined-union, string, or number.

This is not something I'd want to lint against 🤔.

return query || ''; // I want the return type to always be a `string`, the rule expects a `boolean`.

ERROR: src/app/store/features/search/selectors.ts[37, 21]: This type is not allowed in the operand for the '||' operator because it is always falsy. Allowed types are boolean, null-union, undefined-union, string, or number.

[forabi]

no-implicit-coercion on the other hand only caught one pattern:

export const isHomePage = createSelector(
  getRoutingState,
  ({ location }) => !!location && location.pathname === '/',
);

src/app/store/features/url/selectors.ts
   8:21  error  use `Boolean(location)` instead  no-implicit-coercion
  13:21  error  use `Boolean(location)` instead  no-implicit-coercion

I don't think this is very useful because the return type is still boolean, if it wasn't, TypeScript would have caught this where it's used.

[wholesomedev]

Yeah, I'm not sure what the best way forward is in this case.

All we really need is a rule that prevents us from potentially rendering 0, so

{potentiallyZero && <Element/> /* <= lint error */}
{noWayWillbeZero && <Element/> /* <= ok */}

[wholesomedev]

For now there isn't an easy way to implement this. Basically, we'll have to write our own TSLint rule, which is not something we're well positioned to do at the moment.