-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(upload): support custom request #1474
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/tusimple/naive-ui/E69GawvJgywCd1E8fD6CGzfVHqb4 |
Codecov Report
@@ Coverage Diff @@
## main #1474 +/- ##
==========================================
- Coverage 64.88% 64.71% -0.17%
==========================================
Files 886 886
Lines 17198 17181 -17
Branches 4072 4059 -13
==========================================
- Hits 11159 11119 -40
- Misses 5278 5299 +21
- Partials 761 763 +2
Continue to review full report at Codecov.
|
package.json
Outdated
@@ -121,6 +121,7 @@ | |||
"@types/lodash": "^4.14.170", | |||
"@types/lodash-es": "^4.17.4", | |||
"async-validator": "^4.0.1", | |||
"axios": "^0.24.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
能用 fetch 么?不想引入个依赖
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetch没有 onProgress 的功能
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
得依赖 xhr 而且大部分还都用的 axios,要不我在这个demo里面引一个axios 的 script 标签?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
那也应该是放进 devDeps。
https://rollupjs.org/guide/en/#outputmanualchunks
研究一下这个,把 axios 单独开一个 chunk 出来(试试,未必最终要这么做,试好了先提上来)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个放到了devDeps里面了,也单独拆了chunk
src/upload/src/interface.ts
Outdated
onProgress?: (e: ProgressEvent, file: FileInfo) => void | ||
onFinish?: (e: ProgressEvent, file: FileInfo) => void | ||
onError?: (e: ProgressEvent, file: FileInfo) => void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
该必传的得必传,不要让用户判断空
2fe4a63
to
f7b5406
Compare
好了 @07akioni ,不过 axios 引入得问题还得讨论一下 |
package.json
Outdated
@@ -121,6 +121,7 @@ | |||
"@types/lodash": "^4.14.170", | |||
"@types/lodash-es": "^4.17.4", | |||
"async-validator": "^4.0.1", | |||
"axios": "^0.24.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
那也应该是放进 devDeps。
https://rollupjs.org/guide/en/#outputmanualchunks
研究一下这个,把 axios 单独开一个 chunk 出来(试试,未必最终要这么做,试好了先提上来)
src/upload/src/interface.ts
Outdated
withCredentials?: boolean | ||
headers?: FuncOrRecordOrUndef | ||
onProgress: (e: CustomUploadProgressEvent) => void | ||
onFinish: (body?: T) => void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onFinish: (body?: T) => void | |
onFinish: (payload?: T) => void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
考虑一下 body 存在的必要性,我没看出来
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on-finish 事件
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里是把用户传进来的又传回去了?我觉得🈚️必要的样子
src/upload/src/Upload.tsx
Outdated
const event = { | ||
body | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
为啥需要这个东西?body 是用户传来的,这样会搞乱 change 回调的类型,如果真的非常需要的话让用户用闭包做就好了。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inst.onFinish 需要 event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个 event 可以传出去个空的,但是没必要用户传进来我们再丢出去吧?这样类型也不好看
昨天 review 完忘了提交了,尴尬 |
f7b5406
to
0ed31b5
Compare
package.json
Outdated
@@ -109,7 +109,8 @@ | |||
"typescript": "^4.4.2", | |||
"vite": "^2.1.3", | |||
"vue": "^3.2.12", | |||
"vue-router": "^4.0.5" | |||
"vue-router": "^4.0.5", | |||
"axios": "^0.24.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
devDeps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个没解决呢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个放到了devDeps里面了,也单独拆了chunk
data?: FuncOrRecordOrUndef | ||
withCredentials?: boolean | ||
headers?: FuncOrRecordOrUndef | ||
onProgress: (e: {percent: number}) => void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onProgress: (e: {percent: number}) => void | |
onProgress: (e: { percent: number }) => void |
src/upload/src/interface.ts
Outdated
headers?: FuncOrRecordOrUndef | ||
onProgress: (e: CustomUploadProgressEvent) => void | ||
onFinish: () => void | ||
onError: (error: ProgressEvent) => void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我感觉这里也不是必须要 error 这个参数,这个 onError 是我们传给用户的,我们依赖用户的从 onError 传回来的东西吗?感觉并不需要
@JiwenBai |
a742c41
to
6ed7cb8
Compare
Co-authored-by: 07akioni <07akioni2@gmail.com>
Co-authored-by: 07akioni <07akioni2@gmail.com>
好了 @07akioni |
export interface CustomUploadProgressEvent { | ||
percent: number | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export interface CustomUploadProgressEvent { | |
percent: number | |
} |
method, | ||
customRequest | ||
} = options | ||
const { doChange, XhrMap } = options.inst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XhrMap 要这个干啥子呢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这不是自定义逻辑吗?
完整改动参考 ddb2326 |
closes: #1389
先提一下,感觉 customRequest里面的参数需要再讨论一下