Skip to content
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions docs/demo/list-unmount.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
## list

<code src="../examples/list-unmount.tsx"></code>
56 changes: 56 additions & 0 deletions docs/examples/list-unmount.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import React, { useState } from 'react';
import Form from 'rc-field-form';
import Input from './components/Input';
import LabelField from './components/LabelField';

const Demo = () => {
const [form] = Form.useForm();
const [isShow, setIsShow] = useState(true);

return (
<div>
<Form
form={form}
onFinish={values => {
console.log(JSON.stringify(values, null, 2));
console.log(JSON.stringify(form.getFieldsValue({ strict: true }), null, 2));
}}
initialValues={{
users: [
{ name: 'a', age: '1' },
{ name: 'b', age: '2' },
],
}}
>
<Form.Field shouldUpdate>{() => JSON.stringify(form.getFieldsValue(), null, 2)}</Form.Field>

<Form.List name="users">
{fields => {
return (
<div>
{fields.map(field => (
<div key={field.key} style={{ display: 'flex', gap: 10 }}>
<LabelField name={[field.name, 'name']}>
<Input />
</LabelField>
{isShow && (
<LabelField name={[field.name, 'age']}>
<Input />
</LabelField>
)}
</div>
))}
</div>
);
}}
</Form.List>
<button type="button" onClick={() => setIsShow(c => !c)}>
隐藏
</button>
<button type="submit">Submit</button>
</Form>
</div>
);
};

export default Demo;
14 changes: 11 additions & 3 deletions src/useForm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,7 @@ export class FormStore {
const namePathList: InternalNamePath[] | undefined = provideNameList
? nameList.map(getNamePath)
: [];
const noListNamePathList = [...namePathList];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里体感会有点怪,让 AI 帮忙加一点注释说明一下是干什么的以及如果没有 namePathList 的时候会进行填充。
另外语义上这里不应该叫 noListNamePathList,而是应该是类似 finalValueNamePathList 之类的表示用于数据收集的命名。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

提交了


// Collect result in promise list
const promiseList: Promise<FieldError>[] = [];
Expand All @@ -921,9 +922,17 @@ export class FormStore {
const { recursive, dirty } = options || {};

this.getFieldEntities(true).forEach((field: FieldEntity) => {
const fieldNamePath = field.getNamePath();

// Add field if not provide `nameList`
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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gemini-code-assist 你说这里有性能损耗,那有什么具体的代码修改建议吗?

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 包含更复杂的逻辑(例如前缀匹配),则此优化可能需要进一步调整。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我记得代码中好像有 path format 为 string 的函数,帮忙复用一下?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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 的函数,帮忙复用一下?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

感谢你的提问,这让我有机会重新审视之前的优化建议。

你提到代码中是否有 path formatstring 的函数。在当前代码中,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 很可能实现了更复杂的逻辑,例如前缀匹配(即如果 namefieldNamePath 的父路径,也视为匹配)。

如果 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 树)或重新设计匹配逻辑,但这超出了当前简单代码建议的范畴。

) {
noListNamePathList.push(fieldNamePath);
}
namePathList.push(fieldNamePath);
}
Comment on lines 929 to 937

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

这块逻辑虽然可以正常工作,但存在一些可维护性和性能上的问题:

  1. 依赖顺序: 当前实现依赖于 fieldEntities 的处理顺序,即子字段必须在 Form.List 容器之前被处理。这种隐式依赖会使代码难以理解和维护。
  2. 性能: 在 forEach 循环中调用 namePathList.some(),导致时间复杂度为 O(N^2),其中 N 是字段数量。对于包含大量字段的表单,这可能会成为性能瓶颈。

建议重构此部分逻辑,以消除对顺序的依赖并提高性能。例如,可以分两步处理:

  1. 第一次遍历 getFieldEntities(true),收集所有字段路径,并单独识别出哪些是 Form.List 的路径。
  2. 第二次遍历或使用过滤,根据收集到的信息来构建 noListNamePathList,显式地排除那些拥有子字段的 Form.List 路径。

这样做可以使逻辑更清晰,也更健壮。


// Skip if without rule
Expand All @@ -936,7 +945,6 @@ export class FormStore {
return;
}

const fieldNamePath = field.getNamePath();
validateNamePathList.add(fieldNamePath.join(TMP_SPLIT));

// Add field validate rule in to promise list
Expand Down Expand Up @@ -1000,7 +1008,7 @@ export class FormStore {
const returnPromise: Promise<Store | ValidateErrorEntity | string[]> = summaryPromise
.then((): Promise<Store | string[]> => {
if (this.lastValidatePromise === summaryPromise) {
return Promise.resolve(this.getFieldsValue(namePathList));
return Promise.resolve(this.getFieldsValue(noListNamePathList));
}
return Promise.reject<string[]>([]);
})
Expand Down
96 changes: 95 additions & 1 deletion tests/list.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import React, { useState } from 'react';
import { fireEvent, render, act } from '@testing-library/react';
import { resetWarned } from '@rc-component/util/lib/warning';
import Form, { Field, List } from '../src';
Expand Down Expand Up @@ -937,4 +937,98 @@ describe('Form.List', () => {

expect(formRef.current!.getFieldValue('list')).toEqual([{ user: '1' }, { user: '3' }]);
});

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' }] });
});
Comment on lines +941 to +1002

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This is a good test case for the unmount scenario. Consider adding comments within the test to explain the different steps and assertions, making it easier to understand the test's purpose and logic.

Comment on lines +941 to +1002
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

修复 createRef 赋值导致的编译错误

React.createRef() 返回的 RefObjectcurrent 属性在类型层面是只读的,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.

Suggested change
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.


it('list rules', async () => {
const onFinishFailed = jest.fn();

const Demo = () => {
return (
<Form onFinishFailed={onFinishFailed}>
<Form.List name="users" rules={[{ validator: () => Promise.reject('error') }]}>
{fields => {
return fields.map(field => (
<InfoField name={[field.name, 'name']} key={field.key}>
<Input />
</InfoField>
));
}}
</Form.List>
<button type="submit" data-testid="submit">
Submit
</button>
</Form>
);
};

const { queryByTestId } = render(<Demo />);
fireEvent.click(queryByTestId('submit'));
await act(async () => {
await timeout();
});

expect(onFinishFailed).toHaveBeenCalled();
});
});