Skip to content

feat(antd): ArrayTable/ArrayItems sortable impl replace react-sortable-hoc to @dnd-kit - #3855

Merged
janryWang merged 1 commit into
alibaba:formily_nextfrom
charlzyx:feature/antd-sortable
Jun 28, 2023
Merged

feat(antd): ArrayTable/ArrayItems sortable impl replace react-sortable-hoc to @dnd-kit#3855
janryWang merged 1 commit into
alibaba:formily_nextfrom
charlzyx:feature/antd-sortable

Conversation

@charlzyx

@charlzyx charlzyx commented Jun 12, 2023

Copy link
Copy Markdown
Contributor

Before submitting a pull request, please make sure the following is done...

  • Ensure the pull request title and commit message follow the Commit Specific in English.
  • Fork the repo and create your branch from master or formily_next.
  • If you've added code that should be tested, add tests!
  • If you've changed APIs, update the documentation.
  • Ensure the test suite passes (npm test).
  • Make sure your code lints (npm run lint) - we've done our best to make sure these rules match our internal linting guidelines.

Please do not delete the above content


What have you changed?

  1. 修复: @formily/antd 中 ArrayTable/Arrayitems 的拖拽排序底层实现, 将 react-sortable-hoc 替换为 @dnd-kit [Feature Request] antd 依赖的 react-sortable-hoc 库不支持 react@18 #3242
    2. 功能: ArrayTable 组件增加自定义 rowKey 属性支持
    3. 新的排序影响到 Editable 组件的一点逻辑变更

@CLAassistant

CLAassistant commented Jun 12, 2023

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@codecov

codecov Bot commented Jun 12, 2023

Copy link
Copy Markdown

Codecov Report

Patch and project coverage have no change.

Comparison is base (3349815) 96.59% compared to head (18edace) 96.59%.

❗ Current head 18edace differs from pull request most recent head ed5df07. Consider uploading reports for the commit ed5df07 to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##           formily_next    #3855   +/-   ##
=============================================
  Coverage         96.59%   96.59%           
=============================================
  Files               152      152           
  Lines              6695     6695           
  Branches           1810     1810           
=============================================
  Hits               6467     6467           
  Misses              228      228           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@charlzyx charlzyx changed the title feat(antd): ArrayTable react-sortable-hoc to @dnd-kit and rowKey props support WIP: feat(antd): ArrayTable react-sortable-hoc to @dnd-kit and rowKey props support Jun 12, 2023
@charlzyx charlzyx changed the title WIP: feat(antd): ArrayTable react-sortable-hoc to @dnd-kit and rowKey props support feat(antd): ArrayTable/ArrayItems replace react-sortable-hoc to @dnd-kit and ArrayTable rowKey props support Jun 12, 2023
@janryWang

Copy link
Copy Markdown
Collaborator

你的提交邮箱没有用github邮箱,导致CLA这边出问题了

@charlzyx charlzyx changed the title feat(antd): ArrayTable/ArrayItems replace react-sortable-hoc to @dnd-kit and ArrayTable rowKey props support WIP: feat(antd): ArrayTable/ArrayItems replace react-sortable-hoc to @dnd-kit and ArrayTable rowKey props support Jun 15, 2023
@charlzyx
charlzyx force-pushed the feature/antd-sortable branch from fee134f to 7384c52 Compare June 15, 2023 05:41
Comment thread packages/antd/src/editable/index.tsx Outdated
content={props.children}
trigger="click"
destroyTooltipOnHide
// destroyTooltipOnHide

@charlzyx charlzyx Jun 15, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

对应下方问题 2
destroyTooltipOnHide 的情况下, 在 ArrayItems 的例子中,在 Editable.Popover 编辑的 object 对象之后, 会引起下面的问题, 在文档中这一块

<SchemaField.Object
name="config"
x-component="Editable.Popover"
required
title="Configure complex data"
x-reactions={(field) => {
field.title = field.value?.input || field.title
}}
>
<SchemaField.String
x-decorator="FormItem"
required
title="date"
name="date"
x-component="DatePicker.RangePicker"
x-component-props={{ style: { width: '100%' } }}
/>
<SchemaField.String
x-decorator="FormItem"
required
title="input box"
name="input"
x-component="Input"
/>
</SchemaField.Object>
<SchemaField.Void
x-decorator="FormItem"
x-component="ArrayItems.Remove"
/>
</SchemaField.Object>

image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed.

Comment thread packages/antd/src/editable/index.tsx Outdated
return field.setPattern('readPretty')
}
}, [pattern])
}, [pattern, field])

@charlzyx charlzyx Jun 15, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

对应下方问题 1
这段改动是因为:在排序之后, 被 Editable 包裹的 Input 组件的 editable 状态未被重新初始化, 与之前 demo 表现不一致,不改动的表现如下

2023-06-15 11 38 14

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@charlzyx charlzyx changed the title WIP: feat(antd): ArrayTable/ArrayItems replace react-sortable-hoc to @dnd-kit and ArrayTable rowKey props support feat(antd): ArrayTable/ArrayItems replace react-sortable-hoc to @dnd-kit and ArrayTable rowKey props support Jun 15, 2023
@charlzyx

Copy link
Copy Markdown
Contributor Author

你的提交邮箱没有用github邮箱,导致CLA这边出问题了

CLA 修正过了
同时 dnd-kit 对添 Editable 里面有点影响, 需要 re 一下

@janryWang

Copy link
Copy Markdown
Collaborator

你的提交邮箱没有用github邮箱,导致CLA这边出问题了

CLA 修正过了 同时 dnd-kit 对添 Editable 里面有点影响, 需要 re 一下

没太理解,为啥会影响 editable ,之前没问题?

@charlzyx

charlzyx commented Jun 15, 2023

Copy link
Copy Markdown
Contributor Author

你的提交邮箱没有用github邮箱,导致CLA这边出问题了

CLA 修正过了 同时 dnd-kit 对添 Editable 里面有点影响, 需要 re 一下

没太理解,为啥会影响 editable ,之前没问题?

react-sortable-hoc 替换为 @dnt-kit 之后, 新的拖拽跟 Editable 一起使用会有上面截图和评论提到的两个问题;
通过修改 Editable 里面两个地方才能够去掉这两个问题
原因大概是新的 SortableItem 需要拿 index 做 id 和 key 导致了 field 的重新创建, 所以
问题 1. Editable/Input 的 useEditable 需要添加 field 变更的监听, 来完成 Editable 初始化对 field 组件 editable =false 的设置
问题 2. Editable.Popover/Schema 新的拖拽排序完成事件 onDragEnd, field.move 之后, 会触发截图中的报错, 好像是被 Eitable.Popover 包裹的 BaseItem 销毁导致 form 在 each fields 的时候 对应 Editable.Popover 的 field 为空, 这个具体原因我没找到, 但是通过修改 Popover 的 destroyTooltipOnHide 可以修复这个问题

这两个问题是在使用 @dnd-kit 之后出现的, 原来 react-sortable-hoc 没有这个问题, 之前 commit 也有这个问题, 只是没有发现, 在重新整理 commit 做测试的时候发现了, 所以对 Editable 代码做了一些改动

@janryWang

Copy link
Copy Markdown
Collaborator

第一个问题我觉得还好,第二个问题还是再排查一下?我对注释代码,删除代码这种改动特别害怕😨

@charlzyx
charlzyx force-pushed the feature/antd-sortable branch from ac75f2f to f107958 Compare June 19, 2023 02:52
@charlzyx

charlzyx commented Jun 19, 2023

Copy link
Copy Markdown
Contributor Author

第一个问题我觉得还好,第二个问题还是再排查一下?我对注释代码,删除代码这种改动特别害怕😨

emmmm, 修复了,简单来说问题就是 保持 SortableItem id(key) 的稳定性, 顺便问一下 antd-v5 代码怎么找?

@yiyunwan

yiyunwan commented Jun 19, 2023

Copy link
Copy Markdown
Collaborator

https://github.com/formilyjs/antd 这个仓库,你可以看看你这边的问题antd5那边有没有,我那边用的@dnd-kit,不过也没有太过详细的测试,并且性能不算太好

@charlzyx

charlzyx commented Jun 19, 2023

Copy link
Copy Markdown
Contributor Author

https://github.com/formilyjs/antd 这个仓库,你可以看看你这边的问题antd5那边有没有,我那边用的@dnd-kit,不过也没有太过详细的测试,并且性能不算太好

性能是感受?还是有测试能跑?我在 ArrayTable load 10W 数据的 demo 对比了一下, hoc vs dnd-kit 都有肉眼可见的卡顿, 数量少的话不会

@yiyunwan

Copy link
Copy Markdown
Collaborator

数据多了拖拽有明显的卡顿

@yiyunwan

Copy link
Copy Markdown
Collaborator

不过刚看了下antd的,好像也有卡顿,只不过没那么明显

@charlzyx

charlzyx commented Jun 19, 2023

Copy link
Copy Markdown
Contributor Author

不过刚看了下antd的,好像也有卡顿,只不过没那么明显

因吹斯汀,我跑了下 ArrayTable 10W 数据的 Demo , 并 通过火焰图和log 做了一下对比,发现

https://github.com/formilyjs/antd/blob/master/packages/components/src/array-table/index.tsx#L375
这一行,在我的电脑上, move 花费了 2.8s 左右的时间; 而在 antd 里面, 大约是 600ms 左右, 两者的区别我暂时还没有定位到原因;

但针对 antd-v5 demo 中的耗时情况是 @formily/core 中 array.move 方法里面

https://github.com/alibaba/formily/blob/formily_next/packages/core/src/models/ArrayField.ts#L119-L120

这两个 splice 花费了很多时间(初步测试为 splice 导致整个 array 重新 observer 导致), 我尝试用 patch node_modeuls 方法改动了一下, 性能获得了明显提升 (2.8s -> 30ms), 但是问题也很明显, 会导致对应 field 的 observer 语意变更, 这里有个 compare 添加了一些测试用例来说明这个语意变更 https://github.com/alibaba/formily/compare/formily_next...charlzyx:pref/core-array-move-by-slice-repalce?expand=1

Comment thread packages/antd/src/array-items/index.tsx Outdated
// useUniquIdMap 内部使用 WeakMap 来制造唯一 id, 所以
// 如果是简单值, 需要包装为 对象, 这样才能够座位
// WeakMap 的 key 值来使用
const boxedDataSource = dataSource.map((item) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

这个会造成大量的依赖收集信息吧

@janryWang

Copy link
Copy Markdown
Collaborator

第一个问题我觉得还好,第二个问题还是再排查一下?我对注释代码,删除代码这种改动特别害怕😨

emmmm, 修复了,简单来说问题就是 保持 SortableItem id(key) 的稳定性, 顺便问一下 antd-v5 代码怎么找?

weakmap 维护 key,然后通过遍历 dataSource 的方式来收集,这一块逻辑感觉对性能影响比较大

@charlzyx

charlzyx commented Jun 19, 2023

Copy link
Copy Markdown
Contributor Author

第一个问题我觉得还好,第二个问题还是再排查一下?我对注释代码,删除代码这种改动特别害怕😨

emmmm, 修复了,简单来说问题就是 保持 SortableItem id(key) 的稳定性, 顺便问一下 antd-v5 代码怎么找?

weakmap 维护 key,然后通过遍历 dataSource 的方式来收集,这一块逻辑感觉对性能影响比较大

我在参考 v5 的方式,他好像是没用到 id 制造器的,但是发现前置任务 core/array 使用内部使用 splice 导致的性能问题,所以我准备先把性能问题修了,再看 dnd 能不能从 消化一下 v5 抄过来

短视的看的话 可能基于 items 的长度上限可能并不高,table 10w 也根据分页做了 slice 所以实际 weakmap 倒不会特别重,只是要造 id 依赖收集的问题在这个方案里避免不了,10w 实际拖拽卡顿与 hoc 体感一致

@charlzyx
charlzyx force-pushed the feature/antd-sortable branch from f107958 to 73e4818 Compare June 25, 2023 09:57
@charlzyx charlzyx changed the title feat(antd): ArrayTable/ArrayItems replace react-sortable-hoc to @dnd-kit and ArrayTable rowKey props support feat(antd): ArrayTable/ArrayItems sortable impl replace react-sortable-hoc to @dnd-kit Jun 25, 2023
@charlzyx

charlzyx commented Jun 25, 2023

Copy link
Copy Markdown
Contributor Author

第一个问题我觉得还好,第二个问题还是再排查一下?我对注释代码,删除代码这种改动特别害怕😨

emmmm, 修复了,简单来说问题就是 保持 SortableItem id(key) 的稳定性, 顺便问一下 antd-v5 代码怎么找?

weakmap 维护 key,然后通过遍历 dataSource 的方式来收集,这一块逻辑感觉对性能影响比较大

重新实现啦, 这次抄的 antd-v5, 暂时把 Table rowKey 支持移除了, 回头另起 PR 吧

@charlzyx

Copy link
Copy Markdown
Contributor Author

不过刚看了下antd的,好像也有卡顿,只不过没那么明显

fixed by #3863

@charlzyx
charlzyx force-pushed the feature/antd-sortable branch from 18edace to ed5df07 Compare June 26, 2023 03:14
@janryWang
janryWang merged commit b3e270f into alibaba:formily_next Jun 28, 2023
@janryWang

Copy link
Copy Markdown
Collaborator
2023-07-18.18.52.07.mov

两个问题,辛苦修复一下?

  1. 光标样式不对,需要时拖拽样式
  2. 拖拽到底部会频繁跳动

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants