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

chore: onStart always in sync #291

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

chore: onStart always in sync #291

wants to merge 4 commits into from

Conversation

zombieJ
Copy link
Member

@zombieJ zombieJ commented Dec 14, 2020

ref ant-design/ant-design#26536

onStart 改成同步触发,让列表先准备好。

PS:先别合

@vercel
Copy link

vercel bot commented Dec 14, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/react-component/upload/apq9xmpet
✅ Preview: https://upload-git-re-order.react-component.vercel.app

@codecov
Copy link

codecov bot commented Dec 14, 2020

Codecov Report

Merging #291 (b0dc0a8) into master (06a2cf7) will increase coverage by 5.62%.
The diff coverage is 90.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
+ Coverage   73.96%   79.59%   +5.62%     
==========================================
  Files           6        6              
  Lines         242      245       +3     
  Branches       58       58              
==========================================
+ Hits          179      195      +16     
+ Misses         63       50      -13     
Impacted Files Coverage Δ
src/AjaxUploader.tsx 78.35% <90.38%> (+9.65%) ⬆️
src/request.ts 92.30% <0.00%> (+1.92%) ⬆️

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 06a2cf7...74df040. Read the comment docs.

@kerm1it
Copy link
Member

kerm1it commented Dec 15, 2020

这样的话,onStart 会在 transformFile 之前执行,没有问题么?

@zombieJ
Copy link
Member Author

zombieJ commented Dec 15, 2020

逻辑里看 onStart 在 promise 里总是会执行,给予的参数也是原始文件,改后测试也是通的。唯一的问题是这么改,action 如果失败了,它和原来逻辑就不一样了。我在看看 antd 侧有没有办法绕过去。所以这里先 draft 之。

@lgtm-com
Copy link

lgtm-com bot commented Dec 15, 2020

This pull request introduces 1 alert when merging 8982f6b into 953e36a - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@kerm1it
Copy link
Member

kerm1it commented Dec 15, 2020

rebase 一下 master,换了 github actions

@zombieJ
Copy link
Member Author

zombieJ commented Dec 15, 2020

加了一个合成事件 onBatchUpload 用于所有上传触发时触发,但是万一有 file onSuccess 早于 onBatchUpload 触发也是蛋疼的事情。

@lgtm-com
Copy link

lgtm-com bot commented Dec 15, 2020

This pull request introduces 1 alert when merging b0dc0a8 into 06a2cf7 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@zombieJ
Copy link
Member Author

zombieJ commented Dec 15, 2020

干脆 onBatchUpload 触发就是最早的触发,然后 onStart 替换成对应的文件。。。会有点不兼容,感觉需要在 feature 上发。


Update:

也不行,beforeUpload 可以返回 Promise。好蛋疼……

@zombieJ
Copy link
Member Author

zombieJ commented Dec 15, 2020

beforeUpload 本身就支持文件转换,看起来 transformFile 是没有必要的。

@kerm1it
Copy link
Member

kerm1it commented Dec 15, 2020

beforeUpload 本身就支持文件转换,看起来 transformFile 是没有必要的。

两个的用处本身有点不一样,beforeUpload 返回的结果决定是否继续上传,transformFile 返回的时候转换后的新文件.

@zombieJ
Copy link
Member Author

zombieJ commented Dec 15, 2020

beforeUpload 本身就支持文件转换,看起来 transformFile 是没有必要的。

两个的用处本身有点不一样,beforeUpload 返回的结果决定是否继续上传,transformFile 返回的时候转换后的新文件.

嗯,字面上是这么理解。但代码里我发现 beforeUpload 是允许你返回一个新的 file,然后作为替代直接传下去。功能和 transformFile 是一模一样的。

@zombieJ
Copy link
Member Author

zombieJ commented Dec 15, 2020

梳理了一遍,感觉要做大 breaking change 才能一劳永逸了:

  1. transformFile 合并回 beforeUpload
  2. 所有文件 beforeUpload 触发后,一次性 flush onBatchUpload 事件。
    • return false 后做个集合,再跑之后的流程
    • 所有文件触发 onStart 后,再正式启动 post
    • 这样才能保证 onXXX 都可以正确捕获到

@lgtm-com
Copy link

lgtm-com bot commented Dec 16, 2020

This pull request introduces 1 alert when merging 74df040 into 06a2cf7 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

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.

2 participants