Add fast path for React.memo with custom compare#141
Conversation
Greptile OverviewGreptile SummaryThis PR extends the SimpleMemoComponent fast path to support custom Key Changes:
Benefits:
Potential Issue:
Confidence Score: 3/5
Important Files ChangedFile Analysis
|
| const simpleMemoComponentToCompare = new WeakMap< | ||
| Function, | ||
| (objA: mixed, objB: mixed) => boolean, | ||
| >(); |
There was a problem hiding this comment.
logic: WeakMap key collision: if the same function component is wrapped with React.memo using different compare functions (e.g., const A = React.memo(MyFunc, compare1) and const B = React.memo(MyFunc, compare2)), the second call will overwrite the first in the WeakMap. This causes both components to use the same compare function instead of their respective ones.
This is likely rare in practice, but consider using a different key strategy or documenting this limitation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberBeginWork.js
Line: 322:325
Comment:
**logic:** WeakMap key collision: if the same function component is wrapped with `React.memo` using different `compare` functions (e.g., `const A = React.memo(MyFunc, compare1)` and `const B = React.memo(MyFunc, compare2)`), the second call will overwrite the first in the WeakMap. This causes both components to use the same compare function instead of their respective ones.
This is likely rare in practice, but consider using a different key strategy or documenting this limitation.
How can I resolve this? If you propose a fix, please make it concise.|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
|
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
Mirror of facebook/react#34540
Original author: barryam3
Summary
Fixes react/react#33793
This widens the SimpleMemoComponent fast path introduced in react/react#13903 to support memoized function components with custom props comparisons. The fast path eliminates the need for the wrapper MemoComponent to be tracked as a separate Fiber, reducing performance overhead.
In addition, React DevTools will show only one node for the component instead of two (because there is only one Fiber instead of two), which makes debugging easier by reducing visual and therefore mental clutter.
I considered several approaches as discussed in the issue before settling on this one as the best combination of simplicity, safety, and performance after benchmarking and prototyping.
How did you test this change?
Per the contributing instructions:
yarn test- two tests fail ("ReactFlightAsyncDebugInfo › can track async information when awaited" and "ReactFlightDOMEdge › should execute repeated host components only once") but these also fail atmain.yarn test --prod- 59 test failures but again this is the same asmain. Here they are if you want to see: https://gist.github.com/barryam3/82007cf5ef6b1c27621e0395ba9a1e71yarn prettieryarn lintyarn flow dom-node(I first triedyarn flowas recommended by the PR template, but it told me to pick a renderer, anddom-nodeis a good default)Additionally I updated the DevTools unit tests to reflect the component tree in DevTools.
yarn run build-for-devtoolsyarn run test-build-devtools- two tests fail ("ignoreList source map extension › for dev builds › should not ignore list anything" and "ignoreList source map extension › for production builds › should include every source") but these also fail atmainI made the equivalent change in my own application (thousands of components, hundreds memoized with custom arePropsEqual) via a pnpm patch and have not discovered any issues after running in production for about 2 months. I verified that this solves the "extra node" problem in DevTools. (In the interest of sharing any possible caveats, my app is still on React 18.2.0 and I chose a slightly simpler implementation for the patch of just setting a
comparefield on the function instead of using a Weak Map.)