fix(form): stabilize ProForm/BaseForm renders and FormItem memoization#9605
fix(form): stabilize ProForm/BaseForm renders and FormItem memoization#9605chenshuai2144 wants to merge 1 commit intomasterfrom
Conversation
…rativeHandle - ProForm: memoize default contentRender with useCallback to avoid new function identity every render and unnecessary BaseForm content remounts - BaseForm: fix nested JSDoc on formatValues; useImperativeHandle deps use formRef instead of formRef.current; content useMemo depends on formRef - FormItem: useDeepCompareMemo depends on child fieldProps (not duplicate omit() in deps array) for correct deep-compare memoization Co-authored-by: 陈帅 <wasd2144@hotmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates JSDoc comments, refactors dependency arrays in BaseForm and FormItem, and memoizes the contentRender function in ProForm. The review feedback points out a performance regression in FormItem due to improper dependency tracking in useDeepCompareMemo and identifies an unnecessary inclusion of a stable ref object in a useMemo dependency array in BaseForm.
| ['onBlur', 'onChange'], | ||
| ), | ||
| ], | ||
| [childFieldProps], |
There was a problem hiding this comment.
The change to the dependency array of omitOnBlurAndOnChangeProps introduces a performance regression. By using [childFieldProps] instead of the omitted object, the useDeepCompareMemo will now trigger a re-computation whenever onBlur or onChange change (which are often unstable inline functions in React forms). The previous implementation correctly ignored these event handlers by omitting them before the deep comparison, ensuring that only changes to other field props would trigger a re-render of the child component.
| [childFieldProps], | |
| [omit(childFieldProps || {}, ['onBlur', 'onChange'])], |
| } | ||
| return wrapItems; | ||
| }, [grid, RowWrapper, items, contentRender, submitterNode]); | ||
| }, [grid, RowWrapper, items, contentRender, submitterNode, formRef]); |
There was a problem hiding this comment.
Adding formRef to the dependency array of useMemo is unnecessary and potentially misleading. formRef is a ref object created via useRef within the component, so its reference is stable and will not trigger a re-computation of the memoized value. Furthermore, adding a ref object to dependencies does not solve "stale snapshot" issues because useMemo does not track changes to the .current property. If the intent is to ensure the latest form instance is used, accessing formRef.current inside the factory is sufficient, but it won't trigger updates if the ref is updated after the initial render.
| }, [grid, RowWrapper, items, contentRender, submitterNode, formRef]); | |
| }, [grid, RowWrapper, items, contentRender, submitterNode]); |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9605 +/- ##
==========================================
- Coverage 88.53% 88.48% -0.06%
==========================================
Files 368 368
Lines 11257 11259 +2
Branches 4157 4156 -1
==========================================
- Hits 9966 9962 -4
- Misses 1139 1144 +5
- Partials 152 153 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary
Form module review (performance, API shape, tests) and targeted fixes.
Performance
contentRenderwas an inline function, so it was a new reference every parent re-render. That causedBaseForm’scontentuseMemoto recompute and remount the main form body unnecessarily. The default render is now a stableuseCallback.content’suseMemonow listsformRef(the ref object) instead of effectively depending on a staleformRef.currentsnapshot.useImperativeHandlefor the extendedformRefAPI no longer usesformRef.currentin the dependency array (refs should not be read for dependency lists).API / docs
formatValues.validateFieldsReturnFormatValueinBaseForm(stray/**broke the block comment and hurt generated docs/IDE hints).Tests
pnpm run tscandpnpm exec vitest run tests/form(326 tests) pass.