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: Controlled multiple files upload list sync #26612

Merged
merged 3 commits into from
Sep 7, 2020
Merged

fix: Controlled multiple files upload list sync #26612

merged 3 commits into from
Sep 7, 2020

Conversation

zombieJ
Copy link
Member

@zombieJ zombieJ commented Sep 7, 2020

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • Component style update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Internationalization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Other (about what?)

🔗 Related issue link

resolve #26536

💡 Background and solution

📝 Changelog

Language Changelog
🇺🇸 English Fix Upload in control miss file when upload multiple file in same time.
🇨🇳 Chinese 修复 Upload 受控时同时上传多份文件会丢失部分文件的问题。

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

@zombieJ zombieJ requested review from kerm1it and afc163 September 7, 2020 04:23
@@ -147,7 +147,7 @@
"rc-tree": "~3.9.0",
"rc-tree-select": "~4.1.1",
"rc-trigger": "~4.4.0",
"rc-upload": "~3.3.0",
"rc-upload": "~3.3.1",
Copy link
Member

Choose a reason for hiding this comment

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

@ant-design-bot
Copy link
Contributor

ant-design-bot commented Sep 7, 2020

@ant-design-bot
Copy link
Contributor

ant-design-bot commented Sep 7, 2020

@zombieJ
Copy link
Member Author

zombieJ commented Sep 7, 2020

Upload 的逻辑有点蛋疼,多个份文件上传会分次触发 onStart 事件,这个时候虽然触发了 onChange 但是其的 fileList 应该跟着内部状态走。而当最终的 onStart 完成后,其 fileList 则应该跟着 prop 走。useEffect 里触发 fileList 更新改成 raf 到下一帧时同步以防止异步的 onStart 被覆盖的问题。

此外,过去 cc 下对于 fileList 的状态管理异步有些问题,反而导致运行没问题 😂

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2020

Size Change: +209 B (0%)

Total Size: 800 kB

Filename Size Change
./dist/antd-with-locales.min.js 317 kB +106 B (0%)
./dist/antd.min.js 283 kB +103 B (0%)
ℹ️ View Unchanged
Filename Size Change
./dist/antd.compact.min.css 66.2 kB 0 B
./dist/antd.dark.min.css 67.5 kB 0 B
./dist/antd.min.css 66.2 kB 0 B

compressed-size-action

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 7, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3b42561:

Sandbox Source
antd reproduction template Configuration
floral-dawn-5xf2c Issue #26536

@kerm1it
Copy link
Member

kerm1it commented Sep 7, 2020

什么时候会有异步的 onStart ? rc 里面确实有可能是异步的,但是到 antd 这边的话,感觉是同步的,而且 rc 那的 onChange 并不会冒泡到 antd 里面来,所以对 antd 里面来说一个文件最开始的事件就是 onStart

@afc163
Copy link
Member

afc163 commented Sep 7, 2020

/rebase

@zombieJ
Copy link
Member Author

zombieJ commented Sep 7, 2020

主要原因是 onStart 是 promise 下的同时分次触发,使得 onStart 里更新 fileList 会因为闭包导致之后的每个 onStart 都覆盖前一个。因而上一个 fix 通过 ref 来强制保持 fileList 同步并通过 forceUpdate 更新。但是另一个问题是 onStartrc-upload 里是异步触发,使得 React 不能 batch update 而每次 forceUpdate 都触发 useEffect。这让上一次 onChange 没有更新完的 fileListuseEffect 认为受控更新强制覆盖回内部 fileList 导致仍然出现超过 2 个以上的 file 同时上传被覆盖的问题。

这个 fix 做的事情就是把 onStart 作为中间状态,其多次触发的时候,它更新的始终是当前内部维护的 fileList。而 useEffect 由于 raf 每次更新都撤销上一次更新,来确保最后一次更新才会被写入内部的 fileList

@kerm1it
Copy link
Member

kerm1it commented Sep 7, 2020

主要原因是 onStart 是 promise 下的同时分次触发,使得 onStart 里更新 fileList 会因为闭包导致之后的每个 onStart 都覆盖前一个。因而上一个 fix 通过 ref 来强制保持 fileList 同步并通过 forceUpdate 更新。但是另一个问题是 onStartrc-upload 里是异步触发,使得 React 不能 batch update 而每次 forceUpdate 都触发 useEffect。这让上一次 onChange 没有更新完的 fileListuseEffect 认为受控更新强制覆盖回内部 fileList 导致仍然出现超过 2 个以上的 file 同时上传被覆盖的问题。

这个 fix 做的事情就是把 onStart 作为中间状态,其多次触发的时候,它更新的始终是当前内部维护的 fileList。而 useEffect 由于 raf 每次更新都撤销上一次更新,来确保最后一次更新才会被写入内部的 fileList

OK,明白了,瞬间感觉逻辑好复杂呀😂

@codecov
Copy link

codecov bot commented Sep 7, 2020

Codecov Report

Merging #26612 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #26612   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files         383      384    +1     
  Lines        7348     7368   +20     
  Branches     2008     2057   +49     
=======================================
+ Hits         7335     7355   +20     
  Misses         13       13           
Impacted Files Coverage Δ
components/upload/Upload.tsx 100.00% <100.00%> (ø)
components/upload/useFreshState.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9d3ee6...3b42561. Read the comment docs.

@zombieJ
Copy link
Member Author

zombieJ commented Sep 7, 2020

主要就是 transform 支持导致都不是同步的,这个 fix 就是拉齐一下。最好的解法是把 transform 改成支持多个文件的,让 onStart 本身还是同步触发。不过 breaking change,暂时不动它了。

@zombieJ zombieJ merged commit a26517f into master Sep 7, 2020
@zombieJ zombieJ deleted the fix-upload branch September 7, 2020 08:41
@pr-triage pr-triage bot added the PR: merged label Sep 7, 2020
@yoyo837
Copy link
Contributor

yoyo837 commented Sep 7, 2020

rc-upload的不发minor的话,已发布的4.6.x的 在没有当前这个PR的情况下是使用"rc-upload": "~3.3.1",是否有问题?

@zombieJ
Copy link
Member Author

zombieJ commented Sep 7, 2020

rc-upload@3.3.1 的 fix 需要配合这个才生效。原本的该是 bug 还是 bug……

@afc163
Copy link
Member

afc163 commented Sep 7, 2020

@yoyo837 的意思是 rc-upload@3.3.1 + antd@4.6.3 会不会引发额外的问题。

@zombieJ
Copy link
Member Author

zombieJ commented Sep 7, 2020

不会有额外的了,4.6.3fileList 丢文件已经是最大的 bug 了 😂

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.

Upload组件multiple时,onChange的info.fileList数据不正确
5 participants