From 712d1c6fa4025bc40a9d816326f2826fffa42c98 Mon Sep 17 00:00:00 2001 From: Tymoteusz Czech <2625371+Tymek@users.noreply.github.com> Date: Mon, 2 Sep 2024 10:12:09 +0200 Subject: [PATCH] ADR: Use of conditionals in JSX (``) (#8025) Co-authored-by: Melinda Fekete --- website/docs/contributing/ADRs/ADRs.md | 1 + .../ADRs/front-end/jsx-conditionals.md | 121 ++++++++++++++++++ 2 files changed, 122 insertions(+) create mode 100644 website/docs/contributing/ADRs/front-end/jsx-conditionals.md diff --git a/website/docs/contributing/ADRs/ADRs.md b/website/docs/contributing/ADRs/ADRs.md index ff307054b1..b23a6fe6e9 100644 --- a/website/docs/contributing/ADRs/ADRs.md +++ b/website/docs/contributing/ADRs/ADRs.md @@ -43,3 +43,4 @@ We have created a set of ADRs to help guide the development of the front end: * [Preferred folder structure](./front-end/preferred-folder-structure.md) * [Preferred form architecture](./front-end/preferred-form-architecture.md) * [OpenAPI SDK generator](./front-end/sdk-generator.md) +* [Use of conditionals in JSX (refactor of <ConditionallyRender />)](./front-end/jsx-conditionals.md) diff --git a/website/docs/contributing/ADRs/front-end/jsx-conditionals.md b/website/docs/contributing/ADRs/front-end/jsx-conditionals.md new file mode 100644 index 0000000000..9fb3e3cc5f --- /dev/null +++ b/website/docs/contributing/ADRs/front-end/jsx-conditionals.md @@ -0,0 +1,121 @@ +--- +title: "ADR: Use of conditionals in JSX (deprecation of ``)" +--- + +## Background + +Using the `&&` operator in React can lead to unexpected rendering behavior when dealing with certain falsy values. In our codebase, the `` 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 &&

❔

} // will render `NaN` +{0 &&

❔

} // will render `0` +{arr?.length &&

❔

} // 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 +❔

} +/> +``` + +### What's wrong with `` + +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 }) => ( + } + // ❌ 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 `` elements. + +```tsx +
+ This is bad

} + /> + )} + elseShow={'Should be refactored'} + /> +
+``` + +## 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) &&

❔

} // Won't render anything +{!!0 &&

❔

} // 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 ?

👍

: null} // Won't render anything +``` + +It also plays nicely with TypeScript. + +```tsx +export const Test: FC<{ maybeString?: string }> = ({ maybeString }) => + 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 `` 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 `` 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`). + +3. Once all instances of `` have been refactored, remove the component from the codebase.