Skip to content

Conversation

@winddies
Copy link
Contributor

@winddies winddies commented Jun 15, 2020

KODO API 文档 参考

breaking change:

  • 由于 api 的变更及部分结构整理,部分对外接口发生变更:
    • filterParams 删除
    • getHeadersForMkFile 删除
    • createMkFileUrl 删除
    • getResumeUploadedSize 参数发生变化
  • 上传参数增加,需要传 bucket
  • putExtra 参数改变,mimeType 代表的是用户所设置的文件类型; customVars 代替 params
  • 经 PM 确认,断点续传本地存储依赖的文件名由原始文件名更改为fname + key +size

文件目录改变

  • 对原有的 upload 文件进行了拆分,在 upload 文件夹下拆分成 base.ts、index.ts、direct.ts、resume.ts,这里主要是按功能进行结构拆分 。
  • api 请求从 utils 文件抽离,统一放在 api.ts 。

新增功能限制

  • 上传分片数量最大不能超过 10000
  • 每片大小不能超过 1G

对外新增 feature

  • 新增 deleteUploadedChunk,可以删除已上传完成的 chunk
  • next 的数据增加了 uploadInfo 字段
  • config 增加 chunkSize,用户可以设置分片大小
  • putExtra 增加了 metadata 参数,用户可以自定义设置文件元信息

针对上个 #443 (comment) 留下的问题,image 代码暂时不动了,es3 的 plugin 这两天测试后看结果。

@winddies winddies changed the title 对接 new api 对接 KODO 新上传 API Jun 15, 2020
@lzfee0227
Copy link
Collaborator

描述里贴一下上一个 pr 遗留的 TODO @winddies

Copy link
Collaborator

@lzfee0227 lzfee0227 left a comment

Choose a reason for hiding this comment

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

先随便看看

要不要先在现在的 master 或 ts 版本上实现需求 看着也不多
3.0 可以先不着急发?
现在这样改动太多了 风险有点大…
@winddies @nighca

putExtra
);
```
* mimeType: `string`,指定所传的文件类型
Copy link
Collaborator

Choose a reason for hiding this comment

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

不是说兼容吗?不兼容了?

Copy link
Collaborator

Choose a reason for hiding this comment

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

这个搞清楚了在什么情况下能用到了吗?
另外这个不是必填的吧?

@nighca
Copy link
Contributor

nighca commented Jun 16, 2020

先随便看看

要不要先在现在的 master 或 ts 版本上实现需求 看着也不多
3.0 可以先不着急发?
现在这样改动太多了 风险有点大…
@winddies @nighca

是指加新接口实现,老接口先不去动它?

@winddies
Copy link
Contributor Author

这个就是在 ts 版本上修改的,在提 pr 前已经 diff 过了,不过 @lzfee0227 你提的两处的确是diff过程中漏掉的,但是因为这次更改动的是 utils 和 upload,我 diff 过两次,基本上没啥大问题。
至于风险,这个主要在于测试,就目前的改动,我觉得可控。

size: chunk.size
}

utils.setLocalFileInfo(this.key, this.file.size, this.uploadedList)
Copy link
Contributor

Choose a reason for hiding this comment

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

不要每个地方都传一遍 this.key, this.file.size 吧,把 localId 的计算统一做下就好

src/utils.ts Outdated

function createLocalKey(file: File) {
return 'qiniu_js_sdk_upload_file_' + file.name + '_size_' + file.size
function createLocalKey(key: string, size: number) {
Copy link
Contributor

Choose a reason for hiding this comment

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

改为 key + name + size

Copy link
Collaborator

Choose a reason for hiding this comment

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

感觉接口写成 key + File 好一点啊
从 File 里取啥是内部实现

同 setLocalFileInfo 接口

src/utils.ts Outdated

// 文件分块
export function getChunks(file: File, blockSize: number): Blob[] {
const getChunkSize = (chunkSize: number): number => {
Copy link
Contributor

Choose a reason for hiding this comment

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

config.chunkSize 的文档里说一下?

* params: `object`,用来放置自定义变量,变量名必须以 `x:` 开始,自定义变量格式及说明请参考[文档](https://developer.qiniu.com/kodo/manual/1235/vars)
* mimeType: `null || array`,用来限制上传文件类型,为 `null` 时表示不对文件类型限制;限制类型放到数组里:
* fname: `string`,文件原始文件名,若未指定,则魔法变量中无法使用 fname、ext、suffix
* customVars: `object`,用来放置自定义变量,变量名必须以 `x:` 开始,自定义变量格式及说明请参考[文档](https://developer.qiniu.com/kodo/manual/1235/vars)
Copy link
Collaborator

Choose a reason for hiding this comment

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

后面写个 2.x -> 3.x 升级指南?
比如类似 params 换 customVars 这种
其实如果没有 customVars 的时候试图去 params 里取兼容一下也不是不可以考虑……

putExtra
);
```
* mimeType: `string`,指定所传的文件类型
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个搞清楚了在什么情况下能用到了吗?
另外这个不是必填的吧?

src/utils.ts Outdated

function createLocalKey(file: File) {
return 'qiniu_js_sdk_upload_file_' + file.name + '_size_' + file.size
function createLocalKey(key: string, size: number) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

感觉接口写成 key + File 好一点啊
从 File 里取啥是内部实现

同 setLocalFileInfo 接口

README.md Outdated
* customVars: `object`,用来放置自定义变量,变量名必须以 `x:` 开始,自定义变量格式及说明请参考[文档](https://developer.qiniu.com/kodo/manual/1235/vars)
* metadata: `object`,用来防止自定义 meta,变量名必须以 `x-qn-meta-`开始,自定义资源信息格式及说明请参考
[文档](https://developer.qiniu.com/kodo/api/1252/chgm)
* excludeMimeType: `array`,用来限制上传文件类型,在该数组里的文件类型不允许被上传
Copy link
Collaborator

@lzfee0227 lzfee0227 Jun 18, 2020

Choose a reason for hiding this comment

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

mark
breaking change ++
升级文档说一下
以及下面的 mimeType


private updateMkFileProgress(progress: 0 | 1) {
this.loaded.mkFileProgress = progress
this.notifyResumeProgress()
Copy link
Collaborator

Choose a reason for hiding this comment

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

无论 0 还是 1 都 resume ?

}

private getLocalFileInfo() {
return utils.getLocalFileInfo(this.file.name, this.key, this.file.size)
Copy link
Contributor

Choose a reason for hiding this comment

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

#448 (comment) 这里所述,建议把 utils.xxxLocalFileInfo 参数精简为一个 key,而不是三个参数,内部再去拼装成 key

src/utils.ts Outdated

// 文件分块
export function getChunks(file: File, blockSize: number): Blob[] {
const getChunkSize = (chunkSize: number): number => {
Copy link
Contributor

Choose a reason for hiding this comment

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

#448 (comment) 这个还是没有加啊

Copy link
Collaborator

@lzfee0227 lzfee0227 left a comment

Choose a reason for hiding this comment

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

剩 resume 和 utils

Comment on lines +4 to +7
"outDir": "./esm",
"module": "es6",
"lib": ["dom", "es2015", "es2016", "es2017"],
"moduleResolution": "node",
Copy link
Collaborator

Choose a reason for hiding this comment

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

mark

README.md Outdated
### qiniu.getResumeUploadedSize(uploadInfo: object): number
断点续传时返回文件之前已上传的字节数,为 0 代表当前并无该文件的断点信息
### qiniu.deleteUploadedChunks(token: string, key: stting, uploadInfo: object): Promise<void>
删除指定上传任务中已上传完成的片,`key` 为目标文件名`uploadInfo` 可通过 `next` 的返回获取,`token` 由服务端生成
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
删除指定上传任务中已上传完成的片,`key` 为目标文件名`uploadInfo` 可通过 `next` 的返回获取,`token` 由服务端生成
删除指定上传任务中已上传完成的片,`key` 为目标文件名`uploadInfo` 可通过 `next` 的返回获取,`token` 由服务端生成

}

public async putFile() {
public async putFile(): Promise<any> {
Copy link
Contributor

Choose a reason for hiding this comment

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

尽量不要 any?这边应该是能知道返回值类型的吧

}

/** 是否为正整数 */
function isPositiveInteger(n: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

为啥 n 类型是 any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

因为这个函数能接受的参数可以是 any,不用限定类型

Copy link
Contributor

Choose a reason for hiding this comment

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

但是没必要是 any?你如果每个参数都考虑使用者传任意类型的话,那你要改的地方多了去了


private updateMkFileProgress(progress: 0 | 1) {
this.loaded.mkFileProgress = progress
private updateMkFileProgress() {
Copy link
Contributor

Choose a reason for hiding this comment

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

怎么又改回来了?参考 #443 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lzfee0227 觉得这里不用更好

Copy link
Contributor

@nighca nighca Jun 19, 2020

Choose a reason for hiding this comment

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

我也觉得不传更好,我先前评论的意思是

  1. 如果改成不传参,那么函数名就得改下,否则函数名不合适
  2. 这个不是这次引入的改动,所以可以不去动,review 不应该要求人去修改非本次 PR 引入的逻辑

Copy link
Collaborator

Choose a reason for hiding this comment

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

如果 progress 是 0 的时候触发 notifyResumeProgress 好像不太对
所以我倾向于这次重构改了 按 1 的来

message: string // 错误信息,包含错误码,当后端返回提示信息时也会有相应的错误信息。
isRequestError: true | undefined // 用于区分是否 xhr 请求错误当 xhr 请求出现错误并且后端通过 HTTP 状态码返回了错误信息时,该参数为 true否则为 undefined 。
reqId: string // xhr请求错误的 X-Reqid。
code: number /** 请求错误状态码,只有在 err.isRequestError 为 true 的时候才有效。可查阅码值对应说明。*/
Copy link
Contributor

Choose a reason for hiding this comment

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

NIP: 注释在这好像没用?

if (err.code === 401 || err.code === 400) {
this.removeLoalFileInfo()
// uploadId 无效,上传参数有误(多由于本地存储信息的 uploadId 失效
if (err.code === 612 || err.code === 400) {
Copy link
Contributor

Choose a reason for hiding this comment

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

如果是 uploadId 失效导致失败,则一定重试,这个逻辑好像没有做?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个是等 kodo 那边人确认,还没做

const localInfo = utils.getLocalFileInfo(this.getLocalKey())
// 分片必须和当时使用的 uploadId 配套,所以断点续传需要把本地存储的 uploadId 拿出来
// 假如没有 localInfo 本地信息并重新获取 uploadId
if (!localInfo || !localInfo.id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

不需要判断 !localInfo.id 了吧?

const notReachRetryCount = ++this.retryCount <= this.config.retryCount
if (needRetry && notReachRetryCount) {
return this.putFile()
// 如果可以重试或者 uploadId 无效则重新上传
Copy link
Contributor

Choose a reason for hiding this comment

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

uploadId 无效的时候在 resume 里会有一个清除本地数据的操作,那个逻辑配合这边的逻辑 run 才合理,所以建议这边注释也说一下这个事情;不然这边看到就是 uploadId 无效 则无限重试

src/utils.ts Outdated
}

export type CustomError = ResponseError | Error | any
export type CustomError = ResponseError | Error
Copy link
Contributor

Choose a reason for hiding this comment

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

any 不能去?

Copy link
Contributor

@nighca nighca left a comment

Choose a reason for hiding this comment

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

🐺

Copy link
Collaborator

@lzfee0227 lzfee0227 left a comment

Choose a reason for hiding this comment

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

lgtm

@lzfee0227 lzfee0227 merged commit c4214e4 into qiniu:qbs Jun 22, 2020
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