Skip to content
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

fix:delay folder upload until all children files are parsed when drag #561

Merged
merged 8 commits into from
Jul 11, 2024

Conversation

coderz-w
Copy link
Contributor

@coderz-w coderz-w commented Jul 5, 2024

🔗 相关 Issue
ant-design/ant-design#49252
#453
ps:之前提过pr,但是commit记录太乱了,大佬后面也没看就重新开了一个

💡 需求背景和解决方案
当拖拽上传文件夹时,需要手动解析文件树,原代码在解析到子文件为非文件夹时直接执行后续而非等待全部文件解析完成,与普通上传的表现不一致。添加其他变量确保文件全部解析后再进行后续操作
主要改动的地方:
1.traverseFileTree.ts中添加restFile变量记录剩余文件数量,将原本的loopFiles函数移动到了traverseFileTree里便于访问restFile变量
2.原测试中工具函数makeFileSystemEntry逻辑错误,导致当上传的文件夹包含子文件夹时无法进入开始上传逻辑,在测试 https://github.com/react-component/upload/blob/master/tests/uploader.spec.tsx#L377 将webp后缀改成png后测试任然通过可以验证这个bug
c5a1dab39e5f2a6a8c0800e6ddb5d5ac
3.添加了相关测试

Copy link

vercel bot commented Jul 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
upload ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 11, 2024 9:44am

@afc163
Copy link
Member

afc163 commented Jul 5, 2024

@zombieJ 有空看看

src/traverseFileTree.ts Outdated Show resolved Hide resolved
@zombieJ
Copy link
Member

zombieJ commented Jul 10, 2024

感觉计数器这个不太适合,对于文件夹遍历有太多 edge case 容易导致不归零而无法上传。对于过去的实现是即便有失败的,也是能上传。所以如果是要改成 batch flush 的话,我建议是换成队列实现:

  1. 创建一个 progressFileList,入口进入时把 文件/文件夹 置入。创建 flattenFileList,作用如你现有 PR 所示
  2. 写一个 walkFiles 方法,调用时会按照先进先出依次处理文件
    2.1. 如果是文件处理完后则直接加入 flattenFileList
    2.2. 如果是文件夹,将文件置入 progressFileList
  3. 每次处理完,检查当前指针 index 是否为 progressFieList.length 如果是,触发 callback

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.72%. Comparing base (6027d4c) to head (d841736).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #561      +/-   ##
==========================================
+ Coverage   87.21%   88.72%   +1.50%     
==========================================
  Files           6        6              
  Lines         266      275       +9     
  Branches       72       76       +4     
==========================================
+ Hits          232      244      +12     
+ Misses         34       31       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/traverseFileTree.ts Outdated Show resolved Hide resolved
src/traverseFileTree.ts Outdated Show resolved Hide resolved
src/traverseFileTree.ts Outdated Show resolved Hide resolved
src/traverseFileTree.ts Outdated Show resolved Hide resolved
src/traverseFileTree.ts Outdated Show resolved Hide resolved
src/traverseFileTree.ts Outdated Show resolved Hide resolved
src/traverseFileTree.ts Outdated Show resolved Hide resolved
src/traverseFileTree.ts Outdated Show resolved Hide resolved
@zombieJ zombieJ merged commit dfb14cd into react-component:master Jul 11, 2024
7 checks passed
@yoyo837
Copy link
Member

yoyo837 commented Jul 11, 2024

ant-design/ant-design#49252
#453

可以安全的close么?

@coderz-w
Copy link
Contributor Author

ant-design/ant-design#49252 #453

可以安全的close么?

我觉得没啥问题,可以问问豆酱大佬

@yoyo837
Copy link
Member

yoyo837 commented Jul 11, 2024

我觉得没啥问题,可以问问豆酱大佬

小事不用问他,我们专门来代劳小事的。我去关了。

@yoyo837
Copy link
Member

yoyo837 commented Jul 11, 2024

发的 minor 版本,辛苦去antd的feature分支再提交个PR?

@coderz-w
Copy link
Contributor Author

发的 minor 版本,辛苦去antd的feature分支再提交个PR?

听不太懂,要怎么弄呢

@yoyo837
Copy link
Member

yoyo837 commented Jul 11, 2024

发的 minor 版本,辛苦去antd的feature分支再提交个PR?

听不太懂,要怎么弄呢

就是 package.json 版本拉上去

@coderz-w
Copy link
Contributor Author

发的 minor 版本,辛苦去antd的feature分支再提交个PR?

听不太懂,要怎么弄呢

就是 package.json 版本拉上去

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