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

Migrate to dio5.0 #66

Merged
merged 24 commits into from
Apr 18, 2023
Merged

Migrate to dio5.0 #66

merged 24 commits into from
Apr 18, 2023

Conversation

shirne
Copy link
Contributor

@shirne shirne commented Feb 14, 2023

  • Migrade to dio5.0
  • fix lints

This was referenced Feb 14, 2023
@supermebing
Copy link

也没人出来合并这个 然后发布到pub

@shirne shirne force-pushed the dio5 branch 2 times, most recently from 1be1047 to b05b481 Compare February 14, 2023 07:04
@shirne
Copy link
Contributor Author

shirne commented Feb 14, 2023

也没人出来合并这个 然后发布到pub

测试方法:拉取dio分支到本地,修改flutter/pubspec.yaml 中qiniu_sdk_base的引用为path,然后运行flutter/example

qiniu_sdk_base: #^0.5.0
    path: ../base/

git引入的方法: 修改 qiniu_flutter_sdk 的引入方式为git,override qiniu_sdk_base的引入方式为git

dependency_overrides: 
  qiniu_sdk_base:
    git:
      url: https://github.com/shirne/qiniu-sdk
      ref: e3c64afce0d6546e3316a6b3adcd608f58adf81d
      path: base/
dependencies:
   ...
  qiniu_flutter_sdk:
    git:
      url: https://github.com/shirne/qiniu-sdk
      ref: e3c64afce0d6546e3316a6b3adcd608f58adf81d
      path: flutter/

@yinxulai
Copy link
Collaborator

首先真的非常感谢贡献代码
我们将会抽时间尽快审核代码

@huangbinjie
Copy link
Collaborator

我测了下,dio 好像把 Content-Type 的类型的默认值从 application/json 改成 application/x-www-form-urlencoded 了,你可以跑下 base 的测试吗

@shirne
Copy link
Contributor Author

shirne commented Feb 23, 2023

我测了下,dio 好像把 Content-Type 的类型的默认值从 application/json 改成 application/x-www-form-urlencoded 了,你可以跑下 base 的测试吗

我搞一个密钥跑一下看看

@shirne
Copy link
Contributor Author

shirne commented Feb 26, 2023

更新了dio版本到 5.0.1,其它测试通过了,put_bytes 这里还有几个没通过

Expected: StorageErrorType:<StorageErrorType.CANCEL>
  Actual: StorageErrorType:<StorageErrorType.RESPONSE>

应该是上传进度没有回调到导致的

@shirne
Copy link
Contributor Author

shirne commented Feb 26, 2023

@yinxulai @huangbinjie
分片上传初始化的时候返回了这个错误

"{error: incorrect region, please use up-z2.qiniup.com, bucket is: shirne}"

config_test中是通过的,地址是

https://upload-z2.qiniup.com

初始化地址为:

https://upload-na0.qiniup.com/buckets/shirne/objects/dGVzdF9mb3JfcHV0X3BhcnRzLm1wNA==/uploads

@shirne
Copy link
Contributor Author

shirne commented Mar 4, 2023

@yinxulai @huangbinjie
找到问题了,put/helpers下面还需要修改一个域名。

目前用了两个密钥测试都通过了。
22

@huangbinjie
Copy link
Collaborator

为啥升级下 dio 的版本就能解决分片上传的错误...
@yinxulai 来看看把

QiniuError,
StorageError,
StorageErrorType,
PutByPartOptions,
Copy link
Collaborator

Choose a reason for hiding this comment

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

这些删了不就 break 了么

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是需要break的版本的
dio4.x和5.x是breakchange的版本,qiniu-sdk如果不发break版本,会影响到引用了dio的项目

Copy link
Collaborator

Choose a reason for hiding this comment

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

这个 PutByPartOptions 删掉的目的是啥

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shirne 还有这个哈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个之前的版本标记了deprecated。
我搜了下项目里应该也没用到了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@huangbinjie
确认一下这两个要不要删掉,应该是没有用到的了

* Deprecated `PutByPartOptions`
* Deprecated `PutBySingleOptions`

Copy link
Collaborator

@huangbinjie huangbinjie Mar 29, 2023

Choose a reason for hiding this comment

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

@shirne 可以,把这俩文件也删了把。

sdk: ">=2.12.0 <3.0.0"
flutter: ">=1.17.0"
sdk: ">=2.12.0 <4.0.0"
flutter: ">=2.0.0"
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
Contributor Author

Choose a reason for hiding this comment

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

准确地说dio5.x的dart版本要求在 2.15以上
最高版本改为4.0是为了以后能直接兼容dart3.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

为了以后兼容dart3.0考虑的话,flutter也要要求空安全版本

Copy link
Collaborator

@huangbinjie huangbinjie Mar 15, 2023

Choose a reason for hiding this comment

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

准确地说dio5.x的dart版本要求在 2.15以上

dio5 要求 dart 的版本,我们改 flutter 版本干啥呢。另外能确认下 2.12 的 sdk 能跑 dio5 吗,如果不得不发 break 的话版本应该要发 1.0 了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是为dart3.0考虑。3.0是全面空安全,应该不支持非空安全的flutter版本
否则的话到dart3.0的时候还要发一个break

0.x的break可以用小版本号区分

Copy link
Collaborator

Choose a reason for hiding this comment

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

In Dart 3, sound null safety will be, as mentioned and announced earlier, the only supported mode. Pubspec files with an SDK constraint having a lower bound of less than 2.12 will stop resolving in Dart 3 and later. Any source code containing language markers, will fail when you set the constraint to less than 2.12 (e.g. // @Dart=2.9).

看这句话的意思是 Dart3 是支持 Flutter 1.17 的,实在不行就测测把,Dart3 不是已经放出一个版本了么

Copy link
Contributor Author

Choose a reason for hiding this comment

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

改dart版本要自己编译engine的吧,我试了下直接改是匹配不上的

Copy link
Collaborator

Choose a reason for hiding this comment

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

好的,change log 里标注下 breaking 把

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我再测一下flutter版本的兼容性,把这个写准确一点

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dart 2.15 对应的Flutter版本为2.8.0,已测试可以正常运行。
引入的时候需要覆盖flutter_lints ,在readme里加了说明

@shirne
Copy link
Contributor Author

shirne commented Mar 15, 2023

为啥升级下 dio 的版本就能解决分片上传的错误... @yinxulai 来看看把

cfug/dio@42b7eb8
是这个提交影响的,可能跟header有关

@shirne
Copy link
Contributor Author

shirne commented Mar 15, 2023

为啥升级下 dio 的版本就能解决分片上传的错误... @yinxulai 来看看把

cfug/dio@42b7eb8 是这个提交影响的,可能跟header有关

这个问题确认了,那个错误是在complete的post请求中抛出的。之前默认的contextType不正确,在dio5.0.1中自动检测contentType后修复了。
上面的commit 强制指定了contentType,在dio5.0.0中也可以正常运行

@yinxulai
Copy link
Collaborator

@huangbinjie 没大问题我可以先合进去但是先不发布,剩下的我再调整一下

base/analysis_options.yaml Outdated Show resolved Hide resolved
base/lib/qiniu_sdk_base.dart Outdated Show resolved Hide resolved
@@ -183,6 +175,7 @@ class BaseState extends DisposableState<Base> {
}

printToConsole('------------------------');
return PutResponse.fromJson({'error': error});
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
Contributor Author

Choose a reason for hiding this comment

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

This 'onError' handler must return a value assignable to 'PutResponse', but ends without returning a value.

@@ -8,7 +8,7 @@ class UploadPartTask extends RequestTask<UploadPart> {
final int partSize;

// 如果 data 是 Stream 的话,Dio 需要判断 content-length 才会调用 onSendProgress
Copy link
Collaborator

Choose a reason for hiding this comment

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

mark 还有这里,dio 的坑不知道填了没有

Copy link
Contributor Author

Choose a reason for hiding this comment

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

还是这个逻辑的,我下面改的注释地址就是对应代码

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stream类型没有contentLength不报进度也是合理的。数据大小不清楚的情况下读出来取长度,对资源消耗就不可预知了。

@huangbinjie
Copy link
Collaborator

@yinxulai 有空看下为啥测试没触发

@supermebing
Copy link

挺长时间了 还不行?

@yinxulai
Copy link
Collaborator

@yinxulai 有空看下为啥测试没触发

qiniu 组织下的 travis 已经没有续订阅了,现在应该是转向了 actions,我尽快改造成 actions

@huangbinjie
Copy link
Collaborator

#66 (comment) @shirne 这个麻烦回复下

@huangbinjie
Copy link
Collaborator

huangbinjie commented Mar 29, 2023

qiniu 组织下的 travis 已经没有续订阅了,现在应该是转向了 actions,我尽快改造成 actions

@yinxulai 那你得整了啊,不然 master 也不跑

@supermebing
Copy link

每天关注一下 进度

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Patch coverage: 23.43% and project coverage change: -0.15 ⚠️

Comparison is base (3b36dfd) 20.78% compared to head (62ab14d) 20.63%.

❗ Current head 62ab14d differs from pull request most recent head 9ba1d16. Consider uploading reports for the commit 9ba1d16 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
- Coverage   20.78%   20.63%   -0.15%     
==========================================
  Files          30       27       -3     
  Lines         688      693       +5     
==========================================
  Hits          143      143              
- Misses        545      550       +5     
Impacted Files Coverage Δ
base/lib/src/storage/config/host.dart 0.00% <0.00%> (ø)
base/lib/src/storage/error/error.dart 0.00% <0.00%> (ø)
...orage/methods/put/by_part/complete_parts_task.dart 0.00% <0.00%> (ø)
...c/storage/methods/put/by_part/init_parts_task.dart 0.00% <0.00%> (ø)
...rc/storage/methods/put/by_part/put_parts_task.dart 0.00% <0.00%> (ø)
.../storage/methods/put/by_part/upload_part_task.dart 0.00% <0.00%> (ø)
...storage/methods/put/by_part/upload_parts_task.dart 0.00% <0.00%> (ø)
...rage/methods/put/by_single/put_by_single_task.dart 0.00% <ø> (ø)
base/lib/src/storage/methods/put/put_options.dart 0.00% <0.00%> (ø)
base/lib/src/storage/storage.dart 10.71% <ø> (ø)
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@shirne
Copy link
Contributor Author

shirne commented Apr 18, 2023

image
这个冲突怎么搞?先发布一下base的 0.5.0 ?

@yinxulai
Copy link
Collaborator

image 这个冲突怎么搞?先发布一下base的 0.5.0 ?

这个先忽略吧

@@ -14,7 +14,7 @@ dependencies:
qiniu_sdk_base: ^0.5.0

dev_dependencies:
flutter_lints: ^2.0.1
flutter_lints:
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
Contributor Author

Choose a reason for hiding this comment

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

这个不影响代码执行的结果 。
为了适配不同的flutter版本,不指定具体的版本号

Copy link
Collaborator

@huangbinjie huangbinjie left a comment

Choose a reason for hiding this comment

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

lgtm。早点合了把

@yinxulai yinxulai merged commit 3c89d28 into qiniu:master Apr 18, 2023
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