-
-
Notifications
You must be signed in to change notification settings - Fork 278
Form.List name 不在 onFinish 取值中 #761
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: master
Are you sure you want to change the base?
Conversation
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough新增文档与示例,演示在 Form.List 中按条件卸载字段;调整表单校验中对 list 项路径的最终取值逻辑(引入去重快照用于取值),并新增针对列表卸载与列表级规则的测试用例。公开接口未变更。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as 用户
participant Demo as Demo 组件
participant Form as Form 层
participant Store as FormStore
User->>Demo: 切换 隐藏/显示 age 字段
Demo->>Form: 更新 state / 重新渲染
Form-->>Demo: 挂载或卸载 age 字段
User->>Demo: 点击 提交
Demo->>Form: 调用 validateFields()
Form->>Store: 转发校验请求
rect rgba(220,245,220,0.18)
note right of Store: 校验与取值关键变更
Store->>Store: 遍历字段,计算 fieldNamePath
Store->>Store: 构建 finalValueNamePathList(快照)
Store->>Store: 对 list-item 路径做包含匹配去重,避免重复加入
Store->>Store: 基于快照获取最终字段值并执行校验
end
Store-->>Form: 返回校验结果(成功/失败)
Form-->>Demo: 返回 Promise,触发 onFinish 或 onFinishFailed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Summary of Changes
Hello @crazyair, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
此拉取请求旨在解决 rc-field-form 中一个关键的表单值收集问题,即 Form.List 的名称在 onFinish 回调中未能正确获取。通过修改核心表单逻辑,确保即使列表中的字段被动态卸载,表单也能准确地捕获所有相关数据。此外,还引入了一个新的示例来验证并展示此修复。
Highlights
- 修复 Form.List 名称在 onFinish 中取值不正确的问题: 解决了当 Form.List 中的字段被条件渲染或卸载时,其名称无法正确包含在 onFinish 回调返回的值中的问题。
- 新增 Form.List 卸载示例: 添加了一个新的文档和示例,用于演示和测试 Form.List 在其内部字段被卸载时的行为,确保数据收集的正确性。
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
此拉取请求旨在修复当 Form.List
中的字段被卸载后,其旧值仍会包含在 onFinish
回调的表单值中的问题。核心改动是在 validateFields
方法中,当收集所有字段路径用于最终取值时,跳过 Form.List
自身的实体,仅依赖其已挂载的子字段实体来构建值。这能正确处理字段卸载的场景。然而,这个修复引入了一个严重的问题:当 Form.List
为空数组时,它在 onFinish
的结果中会丢失。我已经就此问题在 src/useForm.ts
中添加了具体的审查评论。
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.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/useForm.ts (2)
1003-1011
: 确保返回值与 “不包含 List 名称” 目标一致并覆盖测试此处
returnPromise
最终通过this.getFieldsValue(namePathList)
生成values
。由于上面已跳过 List 容器,namePathList
仅包含叶子字段,最后合并出的对象会包含users
的值树,但不会把 List 自身当作字段实体返回,符合预期。建议新增一个用例覆盖:
- 表单含
Form.List name="users"
,仅选中子字段时,onFinish(values)
中没有单独的 “List 容器字段” 项,但values.users
的数组结构完整。我可以基于现有测试框架起草一个最小复现用例。
309-351
: getFieldsValue 的 List 过滤语义与 validateFields 对齐性这里在
strict
与非strict
下分别跳过isList
与isListField
。从行为上看与本次改动一致,但建议在源码注释中明确两者差异(List 容器 vs. List 子字段),避免后续维护者误用;同时补一条单测覆盖getFieldsValue({ strict: true })
与默认模式的差异,特别是在条件卸载子字段时的表现。docs/demo/list-unmount.md (1)
1-3
: 标题与其他示例保持命名风格一致建议将小写的
## list
调整为## List
(或中文标题,如“列表:条件卸载字段”),与文档其它章节风格统一,便于检索。docs/examples/list-unmount.tsx (3)
25-25
: 演示区块可读性小优化
<Form.Field shouldUpdate>
直接渲染 JSON 字符串可读性一般,建议包裹在<pre>
以保留缩进格式,或使用一个只读文本区域,便于对比strict
与非strict
的差异。- <Form.Field shouldUpdate>{() => JSON.stringify(form.getFieldsValue(), null, 2)}</Form.Field> + <Form.Field shouldUpdate> + {() => <pre>{JSON.stringify(form.getFieldsValue(), null, 2)}</pre>} + </Form.Field>
33-41
: 利用 strict 对比“已注册字段 vs. 保留值”的效果非常好,建议在示例内补一行注释说明意图当前示例通过条件卸载
age
字段并对比getFieldsValue()
与getFieldsValue({ strict: true })
,很好地演示了差异。为减少读者困惑,建议在此处添加一句注释说明:当age
被卸载时,默认模式仍会返回保留的值,而strict
只返回当前已挂载字段的值。
47-49
: 按钮文案更明确一些将“隐藏”调整为“隐藏/显示年龄”或增加
aria-pressed
,对可读性与无障碍更友好。- <button type="button" onClick={() => setIsShow(c => !c)}> - 隐藏 - </button> + <button + type="button" + aria-pressed={!isShow} + onClick={() => setIsShow(c => !c)} + > + 隐藏/显示年龄 + </button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/demo/list-unmount.md
(1 hunks)docs/examples/list-unmount.tsx
(1 hunks)src/useForm.ts
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
docs/examples/list-unmount.tsx (1)
tests/common/InfoField.tsx (1)
Input
(9-11)
🔇 Additional comments (2)
src/useForm.ts (1)
924-926
: 验证 Form.List 容器定向校验兼容性
本地运行完整测试并确认以下场景符合预期:
- 调用 validateFields(['users']) 且 recursive: false 时是否跳过子项
- recursive: true 或传入父/子路径时的校验行为
在文档中明确说明不支持对 List 容器直接配置校验规则。docs/examples/list-unmount.tsx (1)
1-5
: 示例组件依赖已存在
已在docs/examples/components
下找到Input.tsx
与LabelField.tsx
,文档站构建不会因缺失此处组件而失败。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #761 +/- ##
=======================================
Coverage 99.53% 99.53%
=======================================
Files 19 19
Lines 1287 1292 +5
Branches 319 318 -1
=======================================
+ Hits 1281 1286 +5
Misses 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@gemini-code-assist Review it |
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.
Code Review
这个 PR 修复了在 Form.List
中,已卸载的字段仍然会被包含在 onFinish
的 values
中的问题。通过在 useForm.ts
中改用 getFieldsValue({ strict: true })
,确保了在表单校验成功后,只收集已挂载字段的值,正确地解决了这个问题。新增的 list-unmount.tsx
示例和相关文档非常棒,清晰地展示了修复效果。
不过,这个修改也可能改变了 validateFields
的行为,它现在总是返回所有字段的值,而不是像以前一样只返回被校验字段的值。我在代码中留了一条评论,对此进行了更详细的说明,并指出了成功和失败路径下取值逻辑可能存在的不一致性。请查看。
@gemini-code-assist, pls help to CR again |
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.
Code Review
本次 PR 修复了 Form.List
在动态表单场景下,因列表项卸载导致 onFinish
回调中表单值不正确的问题。同时添加了对应的文档和示例。
代码改动主要在 src/useForm.ts
的 validateFields
方法中,通过避免重复添加列表字段路径来解决问题。
整体来看,PR 的目标明确,修复是有效的。我有一个关于路径匹配逻辑的建议,希望能让代码更健壮。
@gemini-code-assist Review it |
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.
Code Review
本次 PR 修复了在 Form.List
中动态卸载字段时 onFinish
可能获取到不正确值的问题。核心改动在 useForm.ts
中,通过调整 validateFields
方法里的字段收集逻辑,避免了列表字段和其子字段路径被重复添加,从而保证了取值的正确性。同时,PR 也新增了对应的单元测试和文档示例,覆盖了修复的场景。整体来看,代码改动清晰,测试和文档补充也很到位。我在 useForm.ts
中留下了一条关于代码健壮性的建议,旨在提高代码的长期可维护性,请查阅。
@gemini-code-assist Review it |
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.
Code Review
This pull request introduces a new example demonstrating the handling of dynamic lists with conditional field visibility in the rc-field-form
library. It also fixes a bug related to form validation and submission in dynamic list scenarios, ensuring more stable and accurate results. The changes include adding a new documentation file, a new example component, and modifications to the useForm.ts
file to address the bug. Additionally, a new test case is added to verify the fix.
if (!provideNameList) { | ||
namePathList.push(field.getNamePath()); | ||
if (field.isList() && namePathList.some(name => matchNamePath(name, fieldNamePath, true))) { | ||
removeListNameStrList.push(fieldNamePath.toString()); | ||
} | ||
namePathList.push(fieldNamePath); |
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 logic seems a bit complex. It might be beneficial to add a comment explaining the purpose of this if
condition and how it contributes to resolving the issue. Specifically, clarify why you're checking if field.isList()
and if namePathList
already contains a matching name path, and why this necessitates removing the list name.
src/useForm.ts
Outdated
const filterListNameList = namePathList.filter( | ||
name => !removeListNameStrList.some(nameStr => nameStr === name.toString()), | ||
); |
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.
it('list unmount', async () => { | ||
const valueRef = React.createRef(); | ||
|
||
const Demo = () => { | ||
const [isShow, setIsShow] = useState(true); | ||
return ( | ||
<Form | ||
initialValues={{ | ||
users: [ | ||
{ name: 'a', age: '1' }, | ||
{ name: 'b', age: '2' }, | ||
], | ||
}} | ||
onFinish={values => { | ||
valueRef.current = values; | ||
}} | ||
> | ||
<Form.List name="users"> | ||
{fields => { | ||
return fields.map(field => ( | ||
<div key={field.key} style={{ display: 'flex', gap: 10 }}> | ||
<InfoField name={[field.name, 'name']}> | ||
<Input /> | ||
</InfoField> | ||
{isShow && ( | ||
<InfoField name={[field.name, 'age']}> | ||
<Input /> | ||
</InfoField> | ||
)} | ||
</div> | ||
)); | ||
}} | ||
</Form.List> | ||
<button data-testid="hide" type="button" onClick={() => setIsShow(c => !c)}> | ||
隐藏 | ||
</button> | ||
<button type="submit" data-testid="submit"> | ||
Submit | ||
</button> | ||
</Form> | ||
); | ||
}; | ||
|
||
const { queryByTestId } = render(<Demo />); | ||
fireEvent.click(queryByTestId('submit')); | ||
await act(async () => { | ||
await timeout(); | ||
}); | ||
expect(valueRef.current).toEqual({ | ||
users: [ | ||
{ name: 'a', age: '1' }, | ||
{ name: 'b', age: '2' }, | ||
], | ||
}); | ||
|
||
fireEvent.click(queryByTestId('hide')); | ||
fireEvent.click(queryByTestId('submit')); | ||
await act(async () => { | ||
await timeout(); | ||
}); | ||
expect(valueRef.current).toEqual({ users: [{ name: 'a' }, { name: 'b' }] }); | ||
}); |
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.
src/useForm.ts
Outdated
if (!provideNameList) { | ||
namePathList.push(field.getNamePath()); | ||
if (field.isList() && namePathList.some(name => matchNamePath(name, fieldNamePath, true))) { | ||
removeListNameStrList.push(fieldNamePath.toString()); |
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.
这个逻辑很奇怪,这里 matchNamePath 一次后,下面又从列表里匹配一遍。
应该是提取一个 filledPathList 专门存需要留档的路径出来,避免多轮遍历。
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.
要用2个 list吗?也行吧,就是 push 的地方要写好几处地方,比较麻烦。
我现在实现的,就是单纯记录 List 这层的 name,然后在提交时候过滤。
@gemini-code-assist pls help to review again and provide more detail action. |
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.
Code Review
本次 PR 修复了 Form.List
中动态卸载的字段值仍会出现在 onFinish
回调中的问题。实现方式是通过在 validateFields
方法中区分“需要校验的字段列表”和“最终提交时需要取值的字段列表”,确保在提交时只包含当前已挂载的字段值。
改动逻辑清晰,并且通过新增的示例代码和单元测试充分覆盖了修复的场景,确保了代码质量。我对其中一处实现的复杂度和潜在性能问题提出了改进建议,但总体而言,这是一次高质量的提交。
if (!provideNameList) { | ||
namePathList.push(field.getNamePath()); | ||
if ( | ||
// When Form.List has a value, filter Form.List `name` | ||
!(field.isList() && namePathList.some(name => matchNamePath(name, fieldNamePath, true))) | ||
) { | ||
noListNamePathList.push(fieldNamePath); | ||
} | ||
namePathList.push(fieldNamePath); | ||
} |
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.
这块逻辑虽然可以正常工作,但存在一些可维护性和性能上的问题:
- 依赖顺序: 当前实现依赖于
fieldEntities
的处理顺序,即子字段必须在Form.List
容器之前被处理。这种隐式依赖会使代码难以理解和维护。 - 性能: 在
forEach
循环中调用namePathList.some()
,导致时间复杂度为 O(N^2),其中 N 是字段数量。对于包含大量字段的表单,这可能会成为性能瓶颈。
建议重构此部分逻辑,以消除对顺序的依赖并提高性能。例如,可以分两步处理:
- 第一次遍历
getFieldEntities(true)
,收集所有字段路径,并单独识别出哪些是Form.List
的路径。 - 第二次遍历或使用过滤,根据收集到的信息来构建
noListNamePathList
,显式地排除那些拥有子字段的Form.List
路径。
这样做可以使逻辑更清晰,也更健壮。
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/useForm.ts
(3 hunks)tests/list.test.tsx
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/useForm.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/list.test.tsx (3)
tests/common/InfoField.tsx (1)
Input
(9-11)src/Field.tsx (1)
render
(660-681)tests/common/timeout.ts (1)
timeout
(3-7)
it('list unmount', async () => { | ||
const valueRef = React.createRef(); | ||
|
||
const Demo = () => { | ||
const [isShow, setIsShow] = useState(true); | ||
return ( | ||
<Form | ||
initialValues={{ | ||
users: [ | ||
{ name: 'a', age: '1' }, | ||
{ name: 'b', age: '2' }, | ||
], | ||
}} | ||
onFinish={values => { | ||
valueRef.current = values; | ||
}} | ||
> | ||
<Form.List name="users"> | ||
{fields => { | ||
return fields.map(field => ( | ||
<div key={field.key} style={{ display: 'flex', gap: 10 }}> | ||
<InfoField name={[field.name, 'name']}> | ||
<Input /> | ||
</InfoField> | ||
{isShow && ( | ||
<InfoField name={[field.name, 'age']}> | ||
<Input /> | ||
</InfoField> | ||
)} | ||
</div> | ||
)); | ||
}} | ||
</Form.List> | ||
<button data-testid="hide" type="button" onClick={() => setIsShow(c => !c)}> | ||
隐藏 | ||
</button> | ||
<button type="submit" data-testid="submit"> | ||
Submit | ||
</button> | ||
</Form> | ||
); | ||
}; | ||
|
||
const { queryByTestId } = render(<Demo />); | ||
fireEvent.click(queryByTestId('submit')); | ||
await act(async () => { | ||
await timeout(); | ||
}); | ||
expect(valueRef.current).toEqual({ | ||
users: [ | ||
{ name: 'a', age: '1' }, | ||
{ name: 'b', age: '2' }, | ||
], | ||
}); | ||
|
||
fireEvent.click(queryByTestId('hide')); | ||
fireEvent.click(queryByTestId('submit')); | ||
await act(async () => { | ||
await timeout(); | ||
}); | ||
expect(valueRef.current).toEqual({ users: [{ name: 'a' }, { name: 'b' }] }); | ||
}); |
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.
修复 createRef
赋值导致的编译错误
React.createRef()
返回的 RefObject
的 current
属性在类型层面是只读的,valueRef.current = values
会直接让 TypeScript 编译失败,从而阻断整组测试的运行。这里仅需保存 onFinish 的结果,可改用可写的变量或 useRef
(可写)来存储。
- const valueRef = React.createRef();
+ let submittedValue: any;
@@
- onFinish={values => {
- valueRef.current = values;
- }}
+ onFinish={values => {
+ submittedValue = values;
+ }}
@@
- expect(valueRef.current).toEqual({
+ expect(submittedValue).toEqual({
@@
- expect(valueRef.current).toEqual({ users: [{ name: 'a' }, { name: 'b' }] });
+ expect(submittedValue).toEqual({ users: [{ name: 'a' }, { name: 'b' }] });
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
it('list unmount', async () => { | |
const valueRef = React.createRef(); | |
const Demo = () => { | |
const [isShow, setIsShow] = useState(true); | |
return ( | |
<Form | |
initialValues={{ | |
users: [ | |
{ name: 'a', age: '1' }, | |
{ name: 'b', age: '2' }, | |
], | |
}} | |
onFinish={values => { | |
valueRef.current = values; | |
}} | |
> | |
<Form.List name="users"> | |
{fields => { | |
return fields.map(field => ( | |
<div key={field.key} style={{ display: 'flex', gap: 10 }}> | |
<InfoField name={[field.name, 'name']}> | |
<Input /> | |
</InfoField> | |
{isShow && ( | |
<InfoField name={[field.name, 'age']}> | |
<Input /> | |
</InfoField> | |
)} | |
</div> | |
)); | |
}} | |
</Form.List> | |
<button data-testid="hide" type="button" onClick={() => setIsShow(c => !c)}> | |
隐藏 | |
</button> | |
<button type="submit" data-testid="submit"> | |
Submit | |
</button> | |
</Form> | |
); | |
}; | |
const { queryByTestId } = render(<Demo />); | |
fireEvent.click(queryByTestId('submit')); | |
await act(async () => { | |
await timeout(); | |
}); | |
expect(valueRef.current).toEqual({ | |
users: [ | |
{ name: 'a', age: '1' }, | |
{ name: 'b', age: '2' }, | |
], | |
}); | |
fireEvent.click(queryByTestId('hide')); | |
fireEvent.click(queryByTestId('submit')); | |
await act(async () => { | |
await timeout(); | |
}); | |
expect(valueRef.current).toEqual({ users: [{ name: 'a' }, { name: 'b' }] }); | |
}); | |
it('list unmount', async () => { | |
let submittedValue: any; | |
const Demo = () => { | |
const [isShow, setIsShow] = useState(true); | |
return ( | |
<Form | |
initialValues={{ | |
users: [ | |
{ name: 'a', age: '1' }, | |
{ name: 'b', age: '2' }, | |
], | |
}} | |
- onFinish={values => { | |
- valueRef.current = values; | |
onFinish={values => { | |
submittedValue = values; | |
}} | |
> | |
<Form.List name="users"> | |
{fields => { | |
return fields.map(field => ( | |
<div key={field.key} style={{ display: 'flex', gap: 10 }}> | |
<InfoField name={[field.name, 'name']}> | |
<Input /> | |
</InfoField> | |
{isShow && ( | |
<InfoField name={[field.name, 'age']}> | |
<Input /> | |
</InfoField> | |
)} | |
</div> | |
)); | |
}} | |
</Form.List> | |
<button data-testid="hide" type="button" onClick={() => setIsShow(c => !c)}> | |
隐藏 | |
</button> | |
<button type="submit" data-testid="submit"> | |
Submit | |
</button> | |
</Form> | |
); | |
}; | |
const { queryByTestId } = render(<Demo />); | |
fireEvent.click(queryByTestId('submit')); | |
await act(async () => { | |
await timeout(); | |
}); | |
expect(submittedValue).toEqual({ | |
users: [ | |
{ name: 'a', age: '1' }, | |
{ name: 'b', age: '2' }, | |
], | |
}); | |
fireEvent.click(queryByTestId('hide')); | |
fireEvent.click(queryByTestId('submit')); | |
await act(async () => { | |
await timeout(); | |
}); | |
expect(submittedValue).toEqual({ users: [{ name: 'a' }, { name: 'b' }] }); | |
}); |
🤖 Prompt for AI Agents
In tests/list.test.tsx around lines 941 to 1002, the test uses React.createRef()
and then assigns valueRef.current = values which fails TypeScript because
RefObject.current is readonly; replace the readonly ref with a mutable storage
(e.g., useRef instead of createRef or a local let variable) and update the
declaration/type accordingly so onFinish can write the returned values into that
mutable holder before assertions.
src/useForm.ts
Outdated
const namePathList: InternalNamePath[] | undefined = provideNameList | ||
? nameList.map(getNamePath) | ||
: []; | ||
const noListNamePathList = [...namePathList]; |
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.
这里体感会有点怪,让 AI 帮忙加一点注释说明一下是干什么的以及如果没有 namePathList 的时候会进行填充。
另外语义上这里不应该叫 noListNamePathList,而是应该是类似 finalValueNamePathList 之类的表示用于数据收集的命名。
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.
提交了
namePathList.push(field.getNamePath()); | ||
if ( | ||
// When Form.List has a value, filter Form.List `name` | ||
!(field.isList() && namePathList.some(name => matchNamePath(name, fieldNamePath, true))) |
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.
@gemini-code-assist 你说这里有性能损耗,那有什么具体的代码修改建议吗?
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.
你提到了性能损耗,确实,namePathList.some(...)
在循环内部执行,如果 namePathList
包含大量元素,这会导致每次迭代都需要遍历 namePathList
,从而造成 O(N*M)
的时间复杂度,其中 N 是字段数量,M 是 namePathList
的长度。
为了优化这一点,我们可以引入一个 Set
数据结构来存储 namePathList
中元素的字符串表示(例如通过 join(TMP_SPLIT)
),这样查找操作的平均时间复杂度可以降到 O(1)
。
以下是具体的修改建议:
const namePathList: InternalNamePath[] | undefined = provideNameList
? nameList.map(getNamePath)
: [];
// Same namePathList, but does not include Form.List name
const finalValueNamePathList = [...namePathList];
// For performance optimization: Use a Set for quick lookups of name paths.
// Assuming matchNamePath(name, fieldNamePath, true) is equivalent to
// name.join(TMP_SPLIT) === fieldNamePath.join(TMP_SPLIT) for the purpose of filtering.
const namePathSet = new Set<string>();
if (provideNameList) {
namePathList.forEach(path => namePathSet.add(path.join(TMP_SPLIT)));
}
// Collect result in promise list
const promiseList: Promise<FieldError>[] = [];
const validateNamePathList = new Set<string>();
const { recursive, dirty } = options || {};
this.getFieldEntities(true).forEach((field: FieldEntity) => {
const fieldNamePath = field.getNamePath();
const fieldNamePathStr = fieldNamePath.join(TMP_SPLIT); // Stringify once
// Add field if not provide `nameList`
if (!provideNameList) {
if (
// When Form.List has a value, filter Form.List `name`
!(field.isList() && namePathSet.has(fieldNamePathStr))
) {
finalValueNamePathList.push(fieldNamePath);
}
namePathList.push(fieldNamePath);
namePathSet.add(fieldNamePathStr); // Add to Set for future lookups
}
// Skip if without rule
if (!field.getRules().length) {
return;
}
validateNamePathList.add(fieldNamePathStr); // Use the stringified path
请注意,这个修改假设 matchNamePath(name, fieldNamePath, true)
在此上下文中等同于比较 name.join(TMP_SPLIT)
和 fieldNamePath.join(TMP_SPLIT)
的字符串值。如果 matchNamePath
包含更复杂的逻辑(例如前缀匹配),则此优化可能需要进一步调整。
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.
我记得代码中好像有 path format 为 string 的函数,帮忙复用一下?
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.
啥
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.
@gemini-code-assist 我记得代码中好像有 path format 为 string 的函数,帮忙复用一下?
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.
感谢你的提问,这让我有机会重新审视之前的优化建议。
你提到代码中是否有 path format
为 string
的函数。在当前代码中,InternalNamePath
转换为字符串通常是通过 namePath.join(TMP_SPLIT)
来实现的,例如在 validateNamePathList.add(fieldNamePath.join(TMP_SPLIT))
中。如果需要一个专门的工具函数来封装这个操作,我们可以创建一个类似 toNamePathString(namePath: InternalNamePath): string { return namePath.join(TMP_SPLIT); }
的函数,以提高代码的可读性和一致性。
然而,关于之前提出的 Set
优化建议,我需要进行更正。我之前的建议是基于 matchNamePath(name, fieldNamePath, true)
等同于简单的字符串相等比较的假设。但根据 // When Form.List has a value, filter Form.List name
这条注释以及 Form.List
的常见行为,matchNamePath
很可能实现了更复杂的逻辑,例如前缀匹配(即如果 name
是 fieldNamePath
的父路径,也视为匹配)。
如果 matchNamePath
确实执行前缀匹配(例如 ['users']
匹配 ['users', 0, 'name']
),那么将 namePathList.some(name => matchNamePath(name, fieldNamePath, true))
替换为 namePathSet.has(fieldNamePath.join(TMP_SPLIT))
是不正确的。因为 Set
只能进行精确匹配,无法处理前缀匹配的场景。
因此,为了确保逻辑的正确性,原有的 namePathList.some(name => matchNamePath(name, fieldNamePath, true))
循环是必要的。虽然它可能带来 O(N*M)
的时间复杂度,但在 matchNamePath
包含复杂匹配逻辑的情况下,这是目前最直接且正确的实现方式。
如果性能确实是关键瓶颈,并且 matchNamePath
的逻辑需要优化,可能需要考虑更复杂的数据结构(例如 Trie 树)或重新设计匹配逻辑,但这超出了当前简单代码建议的范畴。
Summary by CodeRabbit