-
-
Notifications
You must be signed in to change notification settings - Fork 37
Frontend Performance guidelines #159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
438c337 to
f40c93a
Compare
f40c93a to
7546ab5
Compare
| )} | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Ref-Observer Paradox Blocks Image Loading
The AssetCard component observes imgRef.current in the useEffect but never attaches the ref to any DOM element. The <img> tag conditionally renders only after imageLoaded becomes true, but the observer needs the ref attached before it can detect intersection. This creates a chicken-and-egg problem where the image never loads because the observer never fires.
| fetchPrices(chainId, address); | ||
| } | ||
| }, [chainId, address]); // Only when not EVM | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Stale Effects Break Price Fetching Logic
The "Option 2" solution shows two separate effects that both call fetchPrices(chainId, address). The first effect at lines 1057-1060 depends only on [isEvm] but uses chainId and address from the closure without including them in dependencies, creating stale closures. The second effect at lines 1062-1066 depends on [chainId, address] but will never run when isEvm is true. This creates a situation where fetchPrices may be called with stale values and both effects could potentially run simultaneously.
| --- | ||
|
|
||
| --- | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how summary is done, but I would also like that we have chapter titles before these chapter-specific topics begin.
It can be Heading 1, for example starting here as:
# Chapter 3: State ManagementSo, as a reader I could easier navigate and know where I am and in which context. Similar to how books are written and formatted.
| return ( | ||
| <div> | ||
| {tokens.map(token => ( | ||
| <TokenItem key={token.address} token={token} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any advices for the following scenarios:
- What if we don't have unique identifiers?
- How effectively these patterns apply to nested lists and what would be advice for keys in such cases?
- Are constructions similar to this
key={${selectedAlert.key}-detail-${index}}good or bad practice and in which cases these can be possibly good or bad?
Do you think that maybe some of these scenarios are worth mentioning? They do happen across the project and are part of our codebase at present.
|
|
||
| ### Lazy Load Heavy Components | ||
|
|
||
| ```typescript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When these React component examples are provided with JSX, we should probably use jsx syntax for markdown.
Instead of having:
```typescript
We can use:
```jsx
Then it will properly identify the JSX components and paint them in proper colors. It looks like in code editor then, and it's easier to read or identify specific code elements.
I think it properly keeps the TypeScript syntax coloring as well. So, no negative effect on it, I believe.
|
|
||
| ### ❌ Anti-Pattern: Creating Maps/Objects During Render | ||
|
|
||
| Creating Maps, Sets, or complex objects during render blocks the main thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be worth mentioning that we should memoize or change approach for style property also known as inline styles within the JSX components?
I think that one is also very frequent anti-pattern.
Example:
<Text style={{ opacity: shouldShowMainPriceMuted ? loadingOpacity : 1 }}>{whatever}</Text>These things cause object re-creations on every re-render. And, it can also cause re-rendering issues.
Solutions are usually:
- Using
useMemo. - Extracting styles into constants if they're simple.
- Using SCSS, which is probably the most performant way.
P.S. This is causing issues in nested components, and the one that have reasons for re-rendering. It's usually fine if it's used within the page that never have a need for re-rendering (mostly static pages, one time rendered, etc.).
| } | ||
| ``` | ||
|
|
||
| **Note:** For truly expensive functions used across multiple components, consider implementing memoization outside React, as React Compiler only memoizes within components/hooks and doesn't share memoization across components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds a little bit confusing, what's the difference between within components/hooks and across components? Can we have better explanation here?
| - ✅ Tests nullable/optional values before accessing (e.g., enable `strictNullChecks` in TypeScript) | ||
| - ✅ Follows the [Rules of React](https://react.dev/reference/rules) | ||
|
|
||
| React Compiler can verify many Rules of React statically and will **skip compilation** when it detects errors. Install [eslint-plugin-react-compiler](https://www.npmjs.com/package/eslint-plugin-react-compiler) to see compilation errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why advice here is to install the React compiler eslint plugin, and why don't we add it to the project?
|
|
||
| - ✅ **Keep existing `useMemo()` and `useCallback()` calls** - Especially for effect dependencies to ensure behavior doesn't change | ||
| - ✅ **Write new code without `useMemo`/`useCallback`** - Let React Compiler handle it automatically | ||
| - ⚠️ React Compiler will statically validate that auto-memoization matches existing manual memoization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that these are bullet points of some kind, the last one doesn't have a subject and sounds a bit confusing.
It can be probably merged together, something like:
| - ⚠️ React Compiler will statically validate that auto-memoization matches existing manual memoization | |
| - ⚠️ React Compiler will statically validate that auto-memoization matches existing manual memoization, and if it can't prove they're the same, the component/hook is safely skipped over |
| const balances = useSelector(getBalances); | ||
|
|
||
| // Manual memoization required - React Compiler can't optimize Redux values | ||
| const tokensWithBalances = useMemo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be tricky for developers to follow these practices in general. Because, in the previous part we discourage developers to use useMemo since React Compiler will do it for them. And now we encourage them using it because in this case it is not possible for the React Compiler to do the memoization.
I have a feeling that we need to provide better advice for these scenarios. Perhaps we can say that the best practice here is to build memoized selector that will return exact data needed and use it with useSelector. Selectors in the case shown here can be combined. Which means, we can avoid having unnecessary mappings like this one.
There will still probably be issues where it would not be possible to use memoized selectors built with reselect, but will require manual memoization like this. Probably live filtering/search within a component? Maybe we can find more suitable example for this as well.
|
|
||
| **Why:** React Compiler can optimize simple conditionals based on props/state within the same file, but struggles when combined with external state from other files. | ||
|
|
||
| #### 5. Functions Passed to Third-Party Components |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we'll have to do these manual memoizations more frequently, because design system components are moving to @metamask/design-system-react, while component-library is now deprecated. And we're using callbacks on these components probably a lot.
I'm wondering if it's worth mentioning it here? I mean, how this reflects to our current project setup, design system components, and maybe provide some examples, etc. It might help developers to learn and understand this better.
WDYT?
| const filteredTokens = useMemo(() => { | ||
| const inputValue = inputRef.current?.value || filter; | ||
| return tokens.filter(token => token.symbol.includes(inputValue)); | ||
| }, [tokens, filter]); // Note: ref.current not in deps (intentional) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| }, [tokens, filter]); // Note: ref.current not in deps (intentional) | |
| }, [tokens, filter]); // Note: inputRef.current not in deps (intentional) |
| const filteredTokens = useMemo(() => { | ||
| const inputValue = inputRef.current?.value || filter; | ||
| return tokens.filter(token => token.symbol.includes(inputValue)); | ||
| }, [tokens, filter]); // Note: ref.current not in deps (intentional) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add short explanation why this is intentional (why exactly inputRef is omitted from the dependencies)?
|
|
||
| **Why:** Refs are mutable values that React Compiler cannot track statically. DOM queries and other runtime values also cannot be analyzed at compile time. | ||
|
|
||
| #### 7. Reselect Selectors and Complex Compositions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I somehow feel like this is already covered within:
#### 1. Cross-File Dependencies
and
#### 2. Redux Selectors and External State Management
And it might be redundant to have it 🤔
|
|
||
| **Why:** Reselect selectors defined in other files are opaque to React Compiler. However, Reselect already provides memoization, so this is often not an issue. | ||
|
|
||
| #### 8. Effect Dependencies (Keep Existing Memoization) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is somehow repeating introduction from React Compiler Limitations section. I suggest we keep it at one place, more condensed, following DRY principle.
|
|
||
| **Why:** Context providers defined in other files may not be fully analyzed by React Compiler. However, simple context consumption often works fine without manual memoization. | ||
|
|
||
| #### 10. Computations with Multiple Cross-File Dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be too much different from 1. Cross-File Dependencies and few other points already mentioned. It might be redundant.
| ```typescript | ||
| ❌ WRONG: Index keys break reconciliation | ||
| const TokenList = ({ tokens }: TokenListProps) => { | ||
| // If tokens can be reordered/filtered, this breaks React's reconciliation | ||
| return ( | ||
| <div> | ||
| {tokens.map((token, index) => ( | ||
| <TokenItem key={index} token={token} /> // Bad! | ||
| ))} | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| ✅ CORRECT: Use unique, stable identifiers | ||
| const TokenList = ({ tokens }: TokenListProps) => { | ||
| return ( | ||
| <div> | ||
| {tokens.map(token => ( | ||
| <TokenItem key={token.address} token={token} /> // Good! | ||
| ))} | ||
| </div> | ||
| ); | ||
| }; | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it would be easier to read these examples if the titles for wrong and correct are above the code fragment area that is formatted and using syntax highlighting. The issue is that these titles are not part of the code, and they're colored by the syntax highlighter in some way.
Something like this would be more nice, I believe:
❌ WRONG: Index keys break reconciliation
const TokenList = ({ tokens }: TokenListProps) => {
// If tokens can be reordered/filtered, this breaks React's reconciliation
return (
<div>
{tokens.map((token, index) => (
<TokenItem key={index} token={token} /> // Bad!
))}
</div>
);
};✅ CORRECT: Use unique, stable identifiers
const TokenList = ({ tokens }: TokenListProps) => {
return (
<div>
{tokens.map(token => (
<TokenItem key={token.address} token={token} /> // Good!
))}
</div>
);
};|
|
||
| **Recommended libraries:** | ||
|
|
||
| - `react-window` - Lightweight, recommended for most use cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would update this to @tanstack/react-virtual, the one we recently started using for the Tokens list.
- It's underlying library is framework agnostic (
@tanstack/virtual) - both
react-windowandreact-virtualizedare framework specific, and the latter hasn't had significant updates in years
Frontend Performance Optimization Guidelines
Note
Adds a comprehensive React/Redux frontend performance guide covering rendering, hooks/effects, selector patterns, async cleanup, and React Compiler notes with code examples.
docs/frontend-performance.mduseMemo, selectors, web workers, debouncing.JSON.stringify, prevent stale closures/cascades, useuseRefcorrectly, deterministic hook usage.AbortController, clear intervals/subscriptions, avoid large closure retention.Object.values/keyschurn, normalize state, avoid deep traversal/search.Written by Cursor Bugbot for commit 7546ab5. This will update automatically on new commits. Configure here.