feat: support option in onChange handler #589
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Walkthrough该拉取请求对 Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/interface.ts (1)
13-17: 新接口DefaultOptionType完善了树形选择的选项结构这个接口很好地扩展了
BaseOptionType，添加了label、value和children属性，非常适合树形选择组件的需求。使用Omit来定义children属性是一个巧妙的做法，避免了无限类型递归。这个接口直接满足了 PR 的目标，提供了一个可以表示复杂树形数据的结构，包含多个属性。
为了提高可读性，建议添加一个简短的注释来解释
children属性的Omit用法：export interface DefaultOptionType extends BaseOptionType { label?: React.ReactNode; value?: string | number | null; + // 使用 Omit 来防止无限递归 children?: Omit<DefaultOptionType, 'children'>[]; }src/TreeSelect.tsx (3)
49-53: 更新onChange属性类型以支持额外的选项信息
onChange回调函数的类型定义已更新，现在在extra参数中包含了一个可选的option字段，类型为DefaultOptionType。这个改动符合PR的目标，允许在onChange处理器中支持选项信息。建议：
- 请确保更新相关文档，说明这个新的
option字段的用途和内容。- 考虑添加一个代码示例，展示如何使用这个新的
option信息。
418-418: 在triggerChange函数中添加选项信息
triggerChange函数现在在extra参数中包含了option字段，并将其传递给onChange回调。这个改动实现了将选中的选项信息传递给onChange处理器的新功能。建议：
为了提高代码的可读性和可维护性，考虑将ChangeEventExtra & { option?: DefaultOptionType }类型提取为一个单独的类型别名，例如：type EnhancedChangeEventExtra = ChangeEventExtra & { option?: DefaultOptionType };然后在相关的地方使用这个新的类型别名。
Also applies to: 464-465
514-518: 在onOptionSelect函数中传递选项信息
onOptionSelect函数现在在调用triggerChange时包含了option参数。这确保了选中的选项信息能够通过triggerChange函数传递给onChange处理器。建议：
为了保持一致性，考虑将node as DefaultOptionType的类型转换提取为一个单独的变量或函数。例如：const optionNode = node as DefaultOptionType;然后在
triggerChange调用中使用这个变量。这样可以提高代码的可读性，并且如果将来需要对这个转换进行修改，只需要在一个地方进行更改。Also applies to: 548-552
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
tests/__snapshots__/Select.checkable.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/Select.multiple.spec.js.snapis excluded by!**/*.snaptests/__snapshots__/Select.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
- src/TreeSelect.tsx (6 hunks)
- src/interface.ts (1 hunks)
- tests/Select.props.spec.js (3 hunks)
🧰 Additional context used
🔇 Additional comments (7)
src/interface.ts (2)
6-11: 新接口BaseOptionType增强了选项的类型定义这个新的接口为选项类型提供了良好的基础结构。它包含了常用的可选属性如
disabled和className，同时通过索引签名允许添加自定义属性，这提高了接口的灵活性。这个设计很好地符合了 PR 的目标，能够暴露更多的选中选项属性。
6-17: 总体评价：接口更新增强了 TreeSelect 组件的类型系统新添加的
BaseOptionType和DefaultOptionType接口显著提升了 TreeSelect 组件的类型定义。这些变更与 PR 的目标完全一致，为暴露更多选中选项的属性提供了必要的类型支持。这些新接口的设计考虑周全，既提高了类型安全性，又保持了足够的灵活性。特别值得称赞的是，这些更改没有修改现有代码，最大限度地减少了引入破坏性变更的风险。
这些类型定义为实现 PR 中描述的新功能奠定了坚实的基础，同时也提高了代码的可维护性和可读性。
src/TreeSelect.tsx (2)
37-37: 导入新类型DefaultOptionType新导入的
DefaultOptionType类型很可能是为了支持组件的新功能。这个改动看起来是合理的，与PR的目标一致。
37-37: 总体评审意见这些更改很好地实现了将选中的选项传递给 onChange 处理器的新功能。修改集中在组件的相关部分，保持了代码的一致性和可读性。
建议：
- 考虑添加或更新单元测试，以覆盖这个新功能。
- 更新组件的文档，详细说明新的
option参数的用途和结构。- 如果这个改动可能影响性能（例如，在大型树结构中），考虑进行性能测试。
- 检查是否有使用这个组件的其他部分需要更新以适应这个新功能。
为了验证这个改动的影响范围，我们可以运行以下脚本来检查使用了
TreeSelect组件的其他文件：这个脚本将帮助我们识别可能需要更新以适应新的
option参数的文件。Also applies to: 49-53, 418-418, 464-465, 514-518, 548-552
tests/Select.props.spec.js (3)
Line range hint
327-333: 为onChange回调新增option参数的测试在测试参数
arg3中添加了option: expect.anything(),。这确保了在使用不同的showCheckedStrategy时，onChange回调能够正确返回新的option参数。
Line range hint
346-352: 为onChange回调新增option参数的测试与之前的测试用例一致，添加了
option: expect.anything(),，以验证在SHOW_CHILD策略下，onChange回调能够正确返回option参数。
Line range hint
365-371: 为onChange回调新增option参数的测试在
SHOW_PARENT策略的测试中，同样添加了option: expect.anything(),，确保onChange回调正确返回新的option参数。
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #589 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 16
Lines 517 546 +29
Branches 140 163 +23
=========================================
+ Hits 517 546 +29 ☔ View full report in Codecov by Sentry. |
tests/Select.props.spec.js
Outdated
| @@ -324,6 +324,7 @@ describe('TreeSelect.props', () => { | |||
| }), | |||
| ], | |||
| checked: true, | |||
| option: expect.anything(), | |||
There was a problem hiding this comment.
需要填写 treeNode 的额外数据，然后对比这个值是否和 treeNode 一致。不能 anything。
There was a problem hiding this comment.
感谢意见！刚刚对应添加了额外数据进行对比，麻烦看看是否合理。
src/TreeSelect.tsx
Outdated
| @@ -410,7 +415,7 @@ const TreeSelect = React.forwardRef<BaseSelectRef, TreeSelectProps>((props, ref) | |||
| const triggerChange = useRefFunc( | |||
| ( | |||
| newRawValues: SafeKey[], | |||
| extra: { triggerValue?: SafeKey; selected?: boolean }, | |||
| extra: { triggerValue?: SafeKey; selected?: boolean; option?: DefaultOptionType }, | |||
There was a problem hiding this comment.
option 感觉不太好，如果是多选这个就不对了。或许 nodes 会好点。
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
tests/Select.props.spec.js (1)
345-345: 验证新增的 option 参数传递测试用例合理地覆盖了在不同策略（SHOW_ALL、SHOW_CHILD、SHOW_PARENT）下 onChange 回调中 option 参数的传递。建议补充以下场景的测试：
- option 中自定义属性的传递
- 深层嵌套节点的选择场景
testList.forEach(({ strategy, arg1, arg2, arg3 }) => { it(strategy, () => { const handleChange = jest.fn(); + // 添加自定义属性测试 + const customTreeData = { + ...treeData, + customField: 'test', + }; const wrapper = mount( createOpenSelect({ treeCheckable: true, showCheckedStrategy: strategy, onChange: handleChange, + treeData: [customTreeData], }), ); wrapper.selectNode(0); expect(handleChange).toHaveBeenCalledWith(arg1, arg2, expect.anything()); const { triggerNode, allCheckedNodes, ...rest } = handleChange.mock.calls[0][2]; - expect({ ...rest, triggerNode, allCheckedNodes }).toEqual(arg3); + expect({ ...rest, triggerNode, allCheckedNodes }).toEqual({ + ...arg3, + option: expect.objectContaining({ + ...treeData, + customField: 'test', + }), + }); }); });Also applies to: 364-364, 383-383
src/TreeSelect.tsx (1)
464-465: 补充类型文档说明建议为新增的
option/nodes参数添加 JSDoc 文档说明，以便其他开发者更好地理解其用途。const additionalInfo = { preValue: rawLabeledValues, triggerValue, + /** 选中节点的完整数据 */ option: extra.nodes, } as ChangeEventExtra & { option?: DefaultOptionType };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/TreeSelect.tsx(6 hunks)tests/Select.props.spec.js(4 hunks)
🔇 Additional comments (2)
tests/Select.props.spec.js (1)
313-329: 数据结构定义清晰且完整
树形数据结构包含了必要的字段（key、title、value）以及层级关系，这为测试各种策略提供了良好的基础数据。
src/TreeSelect.tsx (1)
514-518: 验证节点类型转换的安全性
在 onOptionSelect 函数中，需要验证节点类型转换的安全性，确保 node as DefaultOptionType 的类型转换是安全的。建议添加类型检查。
Also applies to: 548-552
✅ Verification successful
Let me gather more information about the type safety context by checking the node type and its usage.
Let me check one more aspect about the node parameter's usage in the component.
类型转换是安全的，无需额外类型检查
根据代码分析，node as DefaultOptionType 的类型转换是安全的，原因如下：
DataNode继承自Record<string, any>和TreeDataNodeDefaultOptionType扩展自BaseOptionType，包含基本的选项属性node变量来自keyEntities中的实体，其结构与DefaultOptionType兼容
该类型转换用于事件回调中传递节点信息，不会导致运行时错误。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：检查 DefaultOptionType 的使用情况和可能的类型冲突
# 搜索 DefaultOptionType 的定义和使用
rg "DefaultOptionType" -A 3
# 检查相关接口定义
ast-grep --pattern 'interface $_ {
$$$
DefaultOptionType
$$$
}'
Length of output: 1835
Script:
#!/bin/bash
# 检查 node 变量的类型定义和使用
ast-grep --pattern 'function onOptionSelect($_) {
$$$
}'
# 检查 BaseOptionType 的定义
rg "BaseOptionType" -A 3
# 检查 DataNode 的完整定义
rg "interface DataNode" -A 10
Length of output: 1008
Script:
#!/bin/bash
# 检查 TreeSelect 组件中 node 参数的使用
ast-grep --pattern 'const $_ = ($node: $_) => {
$$$
}'
# 检查 onOptionSelect 的完整实现
rg "onOptionSelect" -A 10 -B 10
Length of output: 3477
| @@ -34,6 +34,7 @@ import type { | |||
| DefaultValueType, | |||
| FieldNames, | |||
| LegacyDataNode, | |||
| DefaultOptionType, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议重命名参数以提高代码清晰度
根据之前的评审意见，建议将 option 重命名为 nodes，因为在多选模式下这个命名更准确。这样可以让接口更清晰地表达其用途。
- extra: ChangeEventExtra & { option?: DefaultOptionType }
+ extra: ChangeEventExtra & { nodes?: DefaultOptionType }Also applies to: 49-53
| @@ -410,7 +415,7 @@ const TreeSelect = React.forwardRef<BaseSelectRef, TreeSelectProps>((props, ref) | |||
| const triggerChange = useRefFunc( | |||
| ( | |||
| newRawValues: SafeKey[], | |||
| extra: { triggerValue?: SafeKey; selected?: boolean }, | |||
| extra: { triggerValue?: SafeKey; selected?: boolean; nodes?: DefaultOptionType }, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
确保类型一致性
为保持代码一致性，建议将 triggerChange 函数中的 option 也更新为 nodes。
- option: extra.nodes,
+ nodes: extra.nodes,Also applies to: 464-465
🤔 This is a ...
🔗 Related Issues
close ant-design/ant-design#50586
💡 Background and Solution
TreeSelect supports option in onChange handler
As sugguested by the member, expose option in the
extraparameter in onChange handlerSummary by CodeRabbit