当region不设置时自动分析上传区域 #346

winddies · 2018-03-06T10:30:25Z

当region不设置时自动分析上传区域
直传进度达到100时请求并未完成的bug
demo里关于图像处理的参数部分去除format参数
config.json.example格式单引号改为双引号
增加eslint格式校验
demo里getUploadUrl改为promise方式，因为之前getUploadUrl是同步的，这里不改会有bug
加入重试，当出现599状态码错误，上传自动重试三次
解决demo中plupload上传的makefile请求的bug

lzfee0227 · 2018-03-07T09:16:08Z

src/utils.js

+  return new Promise((resolve, reject) => {
+    if(config.region != null){
+      let upHosts = regionUphostMap[config.region];
+      let protocol = window.location.protocol === 'https:' ?  "https" : "http";


这里为什么不直接去 protocol 而且要把冒号去掉？
而且下面多处用 protocol 应该提取上去

lzfee0227 · 2018-03-07T09:19:14Z

src/utils.js

+    getUpHosts(token)
+    .then(res => {
+      let hosts = res.data.up.acc.main;
+      let url = window.location.protocol === 'https:' ? "https://" + hosts[0] : "http://" + hosts[0];


这里为什么不直接 `${protocol}//${hosts[0]}`

nighca · 2018-03-07T03:56:47Z

README.md

+
+* 通过源码编译
+
+`·git clone git@github.com:qiniu/js-sdk.git`，进入项目根目录执行`npm install`，执行`npm run build`，即可在dist目录生成qiniu.min.js


中英文间空格（执行npm，在dist目录，生成qiniu）

nighca · 2018-03-07T03:57:32Z

src/upload.js

@@ -84,7 +86,7 @@ export class UploadManager {
      }
      this.stop();
    })
-    return upload;
+    return result


行尾分号？

不应该返回这个 result 啊，这个方法叫 putFile，返回的 promise 就应该代表 put file 这个行为，现在返回的 result 代表的是 get upload url 这个行为

nighca · 2018-03-07T03:57:59Z

src/upload.js

+        if(!this.config.disableStatisticsReport){
+          this.sendLog(res.reqId, 200);
+        }
+      }, err => {})


err => {} 是为什么

原来的结构不用动它的：

let upload = getUploadUrl(this.config, this.token).then(uploadUrl => { this.uploadUrl = res; this.uploadAt = new Date().getTime(); return this.file.size > BLOCK_SIZE ? this.resumeUpload() : this.directUpload(); }); upload.then(...); return upload;

nighca · 2018-03-07T03:58:31Z

test/demo1/main.js

@@ -4,7 +4,7 @@
    var config = {
      useCdnDomain: true,
      disableStatisticsReport: false,
-      region: qiniu.region.z2
+      region: qiniu.region.z2,


无需逗号

nighca · 2018-03-07T03:59:05Z

src/utils.js

+    let uphosts_url = window.location.protocol + "//api.qiniu.com/v2/query?ak=" + putPolicy.ak + "&bucket=" + putPolicy.bucket;
+    return request(uphosts_url, { method: "GET" })
+  }
+  return Promise.reject(err)


err？？

nighca · 2018-03-07T14:03:13Z

src/upload.js

@@ -225,8 +229,14 @@ export class UploadManager {
  }

  updateDirectProgress(loaded, total) {
-    this.progress = {total: this.getProgressInfoItem(loaded, total)}
-    this.onData(this.progress)
+    this.progress = {total: this.getProgressInfoItem(loaded, total + 1)};


这里改动是为啥

这里是我发现在网络比较慢的时候进行直传，进度达到100，但是请求并未完成，那么这个时候不应该进度为100，所以在这里加了个1

那就在注释里说明这个情况，不然维护代码的人无法理解；即便你自己维护，将来你自己也会忘的

nighca · 2018-03-07T14:04:48Z

src/upload.js

      method: "POST",
      body: formData,
      onProgress: (data) => {
        this.updateDirectProgress(data.loaded, data.total);
      },
      onCreate: this.xhrHandler
    });
+    result.then(() => {this.finishDirectProgress()}, err => {});


为什么 err => {}

nighca · 2018-03-07T14:16:35Z

src/utils.js

+      let host = config.useCdnDomain ? upHosts.cdnUphost : upHosts.srcUphost;
+      resolve(`${protocol}//${host}`);
+    }
+    getUpHosts(token)


getUpHosts(token) .then(res => { let hosts = res.data.up.acc.main; return `${protocol}//${hosts[0]}` })

这个 promise 是可以整个被 resolve 的，不用再去它的后边接 resolve & reject

nighca · 2018-03-07T14:18:45Z

src/utils.js

+  try {
+    let putPolicy = JSON.parse(urlSafeBase64Decode(segments[2]));
+    putPolicy.ak = ak;
+    if (putPolicy.scope.indexOf(":") >= 0) {


这里不做判断，直接用

putPolicy.bucket = putPolicy.scope.split(":")[0];

效果是一样的

nighca · 2018-03-07T14:21:36Z

src/utils.js

+    return request(uphosts_url, { method: "GET" })
+  }
+  return Promise.reject(err)
+};


函数声明后面不用分号

nighca · 2018-03-08T06:53:53Z

src/upload.js

      method: "POST",
      body: formData,
      onProgress: (data) => {
        this.updateDirectProgress(data.loaded, data.total);
      },
      onCreate: this.xhrHandler
    });
+    return result.then(res => {


let result = (...) return result.then(...)

跟

return (...).then(...)

有啥区别

nighca · 2018-03-08T06:54:21Z

src/upload.js

-    this.uploadAt = new Date().getTime();
-
-    let upload = this.file.size > BLOCK_SIZE ? this.resumeUpload() : this.directUpload();
+    let upload = getUploadUrl(this.config, this.token).then(res =>{


空格 res =>{ -> res => {

这种低级问题我会记小本本的...

nighca · 2018-03-08T06:59:50Z

src/utils.js

+    putPolicy.bucket = putPolicy.scope.split(":")[0];
+    return putPolicy;
+  } catch(err) {
+    return err;


getPutPolicy 这个方法是预期返回 policy（{ ak, bucket }）的，这里返回一个 err（一般是一个 Error 对象），你让外边怎么用呢？

这个地方可以不用 try catch 的，有错就直接抛出去就好

nighca · 2018-03-08T07:02:54Z

src/utils.js

+    let url = window.location.protocol + "//api.qiniu.com/v2/query?ak=" + putPolicy.ak + "&bucket=" + putPolicy.bucket;
+    return request(url, { method: "GET" })
+  }
+  return Promise.reject(putPolicy);


这边其实是想处理上边 getPutPolicy 时的异常，包装成一个 reject 的 promise，那可以在这个方法里 try catch，或者直接把这些行为在 promise 的构造函数里（new Promise(fn) 的话，发生在 fn 执行中的异常会自动被 catch 且导致对应的 promise 的 reject），即：

try { let putPolicy = getPutPolicy(token); let url = window.location.protocol + "//api.qiniu.com/v2/query?ak=" + putPolicy.ak + "&bucket=" + putPolicy.bucket; return request(url, { method: "GET" }); } catch (e) { return Promise.reject(e); }

或者

return new Promise(resolve => { let putPolicy = getPutPolicy(token); let url = window.location.protocol + "//api.qiniu.com/v2/query?ak=" + putPolicy.ak + "&bucket=" + putPolicy.bucket; resolve(request(url, { method: "GET" })); })

nighca · 2018-03-08T07:05:44Z

src/utils.js

+  let putPolicy = getPutPolicy(token);
+  if(putPolicy.bucket){
+    let url = window.location.protocol + "//api.qiniu.com/v2/query?ak=" + putPolicy.ak + "&bucket=" + putPolicy.bucket;
+    return request(url, { method: "GET" })


分号，小本本 +1

nighca · 2018-03-08T07:06:25Z

src/utils.js

+
+function getUpHosts(token) {
+  let putPolicy = getPutPolicy(token);
+  if(putPolicy.bucket){


if 空格，加个代码格式的 lint 吧，提 PR 前先 lint 通过

nighca · 2018-03-08T07:10:37Z

标题是“当region不设置时自动分析上传区域”，但实际上改动远不止这个，把所有改动在 PR 里列出来说清楚，参考 https://github.com/qbox/portal-base/pull/124

nighca · 2018-03-12T02:09:26Z

src/upload.js

+      if (err.isRequestError){
+        if (err.code === 599){
+          this.retryCount++;
+          this.retryCount > 3 ? "" : this.putFile();


不要用 a ? b : c 的操作符替代 if，影响阅读体验：

if (err.code === 599 && this.retryCount < 3) { this.retryCount++; this.putFile(); }

另外按这边的逻辑，加上重试的话，最多总共会发 4 次，确认下是符合预期的？

nighca · 2018-03-12T02:11:45Z

src/upload.js

        }
+        if (!this.config.disableStatisticsReport){
+          err.code === 0 ? this.sendLog("", -2) : this.sendLog(err.reqId, err.code);


同上，a ? b : c 的操作符组合的结果是一个表达式（expression），不是一个语句（statement），一般代表一个值，而不是一个行为；这里为了达到简洁的目的用这个三元操作符有滥用的嫌疑

nighca · 2018-03-12T02:13:30Z

另外就是，这么多的改动主题，是应该拆成独立的 PR 来做的，而不是放到同一个分支同一个 PR 里，以后注意吧

nighca · 2018-03-12T10:19:09Z

src/upload.js

+            return;
+          }
+          // 重试过程中遇到 599 不需要 onError,当超过最大次数后再 onError
+          this.onError(err)


这行跟下一行都不需要吧？从 if 出来自然就走到 98 行的 onError 了

nighca · 2018-03-12T10:30:49Z

src/upload.js

        } 
+        // 出现 599 重试，最多重试 6 次
+        if (err.code === 599){
+          if (this.retryCount < 6){


这个行为（6）可配置？

已改为可配置

lzfee0227 reviewed Mar 7, 2018

View reviewed changes

zhangheng added 6 commits March 7, 2018 17:33

当region不设置时自动分析上传区域

26c87a0

remove putPolicy key

05a705d

update readme

3b3e463

改善直传进度为100但实际请求并未完成的问题

0c30471

update readme

0fb745c

remove protocol surplus

8fcba2e

winddies force-pushed the sdk-1 branch from 29fe010 to 8fcba2e Compare March 7, 2018 09:36

lzfee0227 previously approved these changes Mar 7, 2018

View reviewed changes

nighca requested changes Mar 7, 2018

View reviewed changes

update

caf2c8d

winddies dismissed lzfee0227’s stale review via caf2c8d March 8, 2018 06:41

zhangheng added 3 commits March 8, 2018 14:44

update utils

7aae895

update Readme

c1215e0

去除图像处理的formate参数

2fb35e6

nighca requested changes Mar 8, 2018

View reviewed changes

zhangheng added 2 commits March 8, 2018 17:21

增加eslint格式校验

6f5a1b8

update

da5551d

nighca previously approved these changes Mar 9, 2018

View reviewed changes

加入重试

40ab270

winddies dismissed nighca’s stale review via 40ab270 March 9, 2018 11:02

zhangheng added 3 commits March 9, 2018 19:34

update

42bd91d

update readme

95a116c

去除多余console

919eeb2

nighca reviewed Mar 12, 2018

View reviewed changes

zhangheng added 2 commits March 12, 2018 18:04

update 重试机制

b4c81bf

update package.json

a1cc3cb

修改标点

1af66de

nighca reviewed Mar 12, 2018

View reviewed changes

重试次数可配置

ba25b62

nighca approved these changes Mar 12, 2018

View reviewed changes

nighca merged commit b0a296b into qiniu:master Mar 12, 2018


		* 通过源码编译

		`·git clone git@github.com:qiniu/js-sdk.git`，进入项目根目录执行`npm install`，执行`npm run build`，即可在dist目录生成qiniu.min.js

当region不设置时自动分析上传区域 #346

当region不设置时自动分析上传区域 #346

Uh oh!

Conversation

winddies commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nighca commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nighca commented Mar 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

winddies commented Mar 6, 2018 •

edited

Loading