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

fix(web): fix 'Future already completed' error when connectTimeout was set. #1550

Closed
wants to merge 3 commits into from

Conversation

ipcjs
Copy link
Contributor

@ipcjs ipcjs commented Aug 24, 2022

fix 'Future already completed' error when connectTimeout was set.

New Pull Request Checklist

This merge request fixes / refers to the following issues:

Pull Request Description

Use Timer instead of Future.delay to implement connectTimeout on Web.

Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

LGTM. The change seems valid for issues.

Are you willing to contribute by letting us pick the PR to another community-maintained dio? (We'll announce the new repo later once we merge PRs as much as possible.)

Comment on lines 80 to 83
} else {
print(
'Warn: connectTimeout is triggered after fetch has completed.');
}
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid print in the production code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this project does not depend on any logging library, so print is used.
How should I modify it? Add if(kDebugModel) or use log()?

Copy link
Member

Choose a reason for hiding this comment

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

Can just ignore it?

Copy link
Member

Choose a reason for hiding this comment

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

Or leaving this with a comment.

Suggested change
} else {
print(
'Warn: connectTimeout is triggered after fetch has completed.');
}
} else {
// connectTimeout is triggered after the fetch has been completed.
}

@AlexV525 AlexV525 mentioned this pull request Oct 20, 2022
6 tasks
Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks. Merging to the fork.

@ipcjs
Copy link
Contributor Author

ipcjs commented Oct 20, 2022

@AlexV525
对了,关于fork的问题。
如果是维护dio的话,感觉最好是想办法联系上@wendux,让他给出维护权限,在已有的仓库上开发。
维护另外一个fork,一方面很多依赖原版dio的项目不一定会迁移,另一方面如果哪天@wendux 又跑回来维护原版dio的话,会造成分裂。我印象中以前就有个dio的fork,后来因为原版dio继续更新,就废弃了。

@ipcjs
Copy link
Contributor Author

ipcjs commented Oct 20, 2022

@AlexV525 Like Hive, it's now looking for a third maintainer 😅
isar/hive#966

@AlexV525
Copy link
Member

AlexV525 commented Oct 21, 2022

@AlexV525 对了,关于fork的问题。 如果是维护dio的话,感觉最好是想办法联系上@wendux,让他给出维护权限,在已有的仓库上开发。 维护另外一个fork,一方面很多依赖原版dio的项目不一定会迁移,另一方面如果哪天@wendux 又跑回来维护原版dio的话,会造成分裂。我印象中以前就有个dio的fork,后来因为原版dio继续更新,就废弃了。

谢谢提醒。关于此问题已经有过非常长久的维护记录为鉴。目前 dio 也有外部的维护者,但最后都因为没有办法受到开源社区的激励而逐渐放弃参与仓库的维护,所以几乎所有的维护者包括原作者都已经失去了维护这个项目的源动力。虽然有我们可以参照的已经失败的分叉,但作为社区的一份子,我们仍然想尝试将像这样的热门 package 维护好,让社区不要总是被背刺。

更加详细的初衷我们会在不久后单独说明,若你能理解那便是我们的荣幸。谢谢!

@jdde
Copy link

jdde commented Nov 30, 2022

Btw, +1 from unping.com to solve this :) would be great to get that into the main branch

@davidmigloz
Copy link

@AlexV525 which "community-maintained dio" fork are you referring to?

@AlexV525
Copy link
Member

@AlexV525 which "community-maintained dio" fork are you referring to?

We'll remind related issues soon as we make the fork public.

@AlexV525
Copy link
Member

AlexV525 commented Dec 18, 2022

Hi everyone! We've made our repo public and published a new version of dio, named diox.
The new package contains the PR of the fix.
Please refer to https://pub.dev/packages/diox/versions/5.0.0-dev.1 to use the fork.
You can also see why we're working for a hardfork at cfug/diox#29 and #1607.

@AlexV525
Copy link
Member

@AlexV525 对了,关于fork的问题。 如果是维护dio的话,感觉最好是想办法联系上@wendux,让他给出维护权限,在已有的仓库上开发。 维护另外一个fork,一方面很多依赖原版dio的项目不一定会迁移,另一方面如果哪天@wendux 又跑回来维护原版dio的话,会造成分裂。我印象中以前就有个dio的fork,后来因为原版dio继续更新,就废弃了。

这次我们永久地(希望是)解决了这个问题,希望未来能够为社区其他的开源项目作好先锋。

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.

None yet

4 participants