1
0
mirror of https://github.com/Unleash/unleash.git synced 2024-12-28 00:06:53 +01:00
unleash.unleash/website/docs/contributing/ADRs/front-end/jsx-conditionals.md

132 lines
4.6 KiB
Markdown
Raw Permalink Normal View History

---
title: "ADR: Use of conditionals in JSX (deprecation of `<ConditionallyRender />`)"
---
## Background
Using the `&&` operator in React can lead to unexpected rendering behavior when dealing with certain falsy values. In our codebase, the `<ConditionallyRender />` component has been used to render React elements based on a boolean condition. However, it has certain drawbacks, which is why we would like to replace it with the ternary operator.
### Pitfalls of `&&` operator
While most truthy and falsy values behave as expected with the `&&` operator, certain falsy values can produce unintended outcomes:
```tsx
{NaN && <p></p>} // will render `NaN`
{0 && <p></p>} // will render `0`
{arr?.length && <p></p>} // can render `0`
```
These issues can cause bugs in components that conditionally render UI elements based on numeric values or other potentially falsy conditions. For this reason, we use a wrapper.
```tsx
<ConditionallyRender
condition={arr?.length}
show={<p></p>}
/>
```
### What's wrong with `<ConditionallyRender />`
While this solves leaking render issues, it has some drawbacks.
#### Poor TypeScript support
```tsx
import type { FC } from 'react';
import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';
const SubComponent: FC<{ text: string }> = ({ text }) => <>{text}</>;
export const Test: FC<{ maybeString?: string }> = ({ maybeString }) => (
<ConditionallyRender
condition={maybeString}
// ❌ TS Error: Type 'string | undefined' is not assignable to type 'boolean'
// You have to use `Boolean(maybeString)`
show={<SubComponent text={maybeString} />}
// ❌ TS Error: Type 'string | undefined' is not assignable to type 'string'
// you have to use `maybeString!` here
/>
);
```
#### Obfuscation of code smells and code cruft
Nested ternaries are easier to spot than nested `<ConditionallyRender />` elements.
```tsx
<div>
<ConditionallyRender
condition={Boolean(a)}
show={(
<ConditionallyRender
condition={Boolean(b)}
show={<p>This is bad</p>}
/>
)}
elseShow={'Should be refactored'}
/>
</div>
```
Nested operator does not look like other JSX components.
```tsx
<div>
{a ? (
b ? <p>This is bad</p> : null
) : 'Should be refactored'}
</div>
```
## Options considered
To avoid these issues, safer alternatives to the `&&` operator can be used:
### **Convert to boolean**
We could try to explicitly convert the condition to a boolean, ensuring that only `true` or `false` determine the rendering.
```tsx
{Boolean(NaN) && <p></p>} // Won't render anything
{!!0 && <p></p>} // Also safe
```
**Unfortunately** Biome (the linter we use) does not include rules to automatically enforce a safer usage of the `&&` operator, as ESLint did.
### Ternary Operator
The ternary operator is a more explicit and safer approach. This covers cases where we need to return `null` or `undefined`.
``` tsx
{NaN ? <p>👍</p> : null} // Won't render anything
```
It also plays nicely with TypeScript.
```tsx
export const Test: FC<{ maybeString?: string }> = ({ maybeString }) =>
maybeString ? <SubComponent text={maybeString} /> : null;
```
This is what we will use from now onwards.
## Consequences
Positive: The codebase will become more type-safe and easier to understand.
Negative: The `<ConditionallyRender />` component is imported in nearly 400 files. Significant refactoring effort is required.
Performance: There was no measurable performance difference between code with and without this component. This was tested on production bundle, on the features search (table) and projects list pages.
## Migration plan
1. Mark `<ConditionallyRender />` as deprecated in the codebase with a clear JSDoc comment.
2. Automated refactoring with AST (Abstract Syntax Tree):
There already is a script developed that can convert files between `ConditionallyRender` and ternary syntax. It uses jscodeshift, a library. It will be put in `frontend/scripts/transform.js`.
3. Each change will have to be reviewed. The order of refactoring should be:
1. New features that are behind feature flags.
2. Non-critical or not in demand pages, like new signals or feedback component.
3. Less complex pages, for example in `/src/component/admin`.
4. More complex and critical pages, like strategy editing.
5. Utilities and components used in many places (`/src/component/common`).
4. Once all instances of `<ConditionallyRender />` have been refactored, remove the component from the codebase.