feat: add getFieldsName API to form instance#787
feat: add getFieldsName API to form instance#787QdabuliuQ wants to merge 1 commit intoreact-component:masterfrom
Conversation
|
@QdabuliuQ is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Walkthrough本次 PR 新增了 ChangesgetFieldsName 方法新增
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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 introduces the getFieldsName method to the FormInstance, allowing users to retrieve the name paths of all registered fields. The implementation includes updates to the FormStore, FieldContext, and TypeScript interfaces, along with new documentation and a comprehensive test suite. A review comment suggests considering whether the method should deduplicate name paths when multiple fields share the same name, as the current implementation returns an entry for every registered field entity.
| private getFieldsName = (): InternalNamePath[] => { | ||
| this.warningUnhooked(); | ||
| return this.getFieldEntities(true).map(entity => entity.getNamePath()); | ||
| }; |
There was a problem hiding this comment.
The current implementation of getFieldsName returns all registered field entities, which may include duplicate name paths if multiple Field components share the same name. While this is consistent with getFieldsError, it might be more useful for consumers of getFieldsName to receive unique name paths, especially if they intend to use these paths with getFieldsValue. Consider if deduplication is desired here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/getFieldsName.test.tsx (1)
77-97: ⚡ Quick win
Form.List测试描述遗漏了列表容器字段本身测试标题
'includes Form.List item fields'暗示只验证子项路径（['list', 0]、['list', 1]），但断言实际上还包含了Form.List内部渲染的Field（name={[]}+prefixName=['list']）所注册的容器路径['list']。建议更新描述以反映完整行为，便于后续维护者理解
getFieldsName()会同时返回列表容器字段：✏️ 建议的描述改法
- it('includes Form.List item fields', () => { + it('includes both Form.List container field and its item fields', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/getFieldsName.test.tsx` around lines 77 - 97, Update the test description to accurately reflect that getFieldsName() returns both the Form.List item paths and the List container path: change the it() title from 'includes Form.List item fields' to something like 'includes Form.List item fields and the List container field' so the test intent matches the assertion involving Form.List, List, and getFieldsName().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/getFieldsName.test.tsx`:
- Around line 42-55: Update the ambiguous test title in the spec that currently
reads "includes one entry per Field with the same name" to explicitly state the
non-deduplication behavior so readers won't misinterpret it as "deduplicates by
name"; locate the test case using the it(...) block that references
formRef.current!.getFieldsName() in tests/getFieldsName.test.tsx and replace the
description with something clearer like "returns one entry per Field instance
even when names are identical" or "does not deduplicate fields with the same
name" to accurately reflect the behavior.
---
Nitpick comments:
In `@tests/getFieldsName.test.tsx`:
- Around line 77-97: Update the test description to accurately reflect that
getFieldsName() returns both the Form.List item paths and the List container
path: change the it() title from 'includes Form.List item fields' to something
like 'includes Form.List item fields and the List container field' so the test
intent matches the assertion involving Form.List, List, and getFieldsName().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c129126-f098-40b4-9f81-d242557e7d00
📒 Files selected for processing (6)
docs/demo/getFieldsName.mddocs/examples/getFieldsName.tsxsrc/FieldContext.tssrc/hooks/useForm.tssrc/interface.tstests/getFieldsName.test.tsx
| it('includes one entry per Field with the same name', () => { | ||
| const formRef = React.createRef<FormInstance>(); | ||
| render( | ||
| <Form ref={formRef}> | ||
| <Field name="x"> | ||
| <Input /> | ||
| </Field> | ||
| <Field name="x"> | ||
| <Input /> | ||
| </Field> | ||
| </Form>, | ||
| ); | ||
| expect(formRef.current!.getFieldsName()).toEqual([['x'], ['x']]); | ||
| }); |
There was a problem hiding this comment.
测试描述存在歧义，建议澄清
'includes one entry per Field with the same name' 可被误读为"同名字段只返回一条"（即去重），而实际行为恰好相反——不做去重，每个注册的 Field 实例均贡献一条记录。建议将描述改为明确表达非去重语义，例如：
✏️ 建议的描述改法
- it('includes one entry per Field with the same name', () => {
+ it('does not deduplicate Fields sharing the same name', () => {📝 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('includes one entry per Field with the same name', () => { | |
| const formRef = React.createRef<FormInstance>(); | |
| render( | |
| <Form ref={formRef}> | |
| <Field name="x"> | |
| <Input /> | |
| </Field> | |
| <Field name="x"> | |
| <Input /> | |
| </Field> | |
| </Form>, | |
| ); | |
| expect(formRef.current!.getFieldsName()).toEqual([['x'], ['x']]); | |
| }); | |
| it('does not deduplicate Fields sharing the same name', () => { | |
| const formRef = React.createRef<FormInstance>(); | |
| render( | |
| <Form ref={formRef}> | |
| <Field name="x"> | |
| <Input /> | |
| </Field> | |
| <Field name="x"> | |
| <Input /> | |
| </Field> | |
| </Form>, | |
| ); | |
| expect(formRef.current!.getFieldsName()).toEqual([['x'], ['x']]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/getFieldsName.test.tsx` around lines 42 - 55, Update the ambiguous test
title in the spec that currently reads "includes one entry per Field with the
same name" to explicitly state the non-deduplication behavior so readers won't
misinterpret it as "deduplicates by name"; locate the test case using the
it(...) block that references formRef.current!.getFieldsName() in
tests/getFieldsName.test.tsx and replace the description with something clearer
like "returns one entry per Field instance even when names are identical" or
"does not deduplicate fields with the same name" to accurately reflect the
behavior.
1 participant
🤔 这个变动的性质是？
🔗 相关 Issue
issues: 57757
💡 需求背景和解决方案
新增 getFieldsName api 获取 Form.Item的name属性值
📝 更新日志
Summary by CodeRabbit
发布说明
新功能
getFieldsName()方法，支持获取表单中所有已注册字段的名称路径，包括嵌套字段和列表字段。文档
测试