-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
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.
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.)
} else { | ||
print( | ||
'Warn: connectTimeout is triggered after fetch has completed.'); | ||
} |
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.
We should avoid print
in the production code.
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.
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()
?
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.
Can just ignore it?
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.
Or leaving this with a comment.
} else { | |
print( | |
'Warn: connectTimeout is triggered after fetch has completed.'); | |
} | |
} else { | |
// connectTimeout is triggered after the fetch has been completed. | |
} |
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.
LGTM. Thanks. Merging to the fork.
@AlexV525 Like Hive, it's now looking for a third maintainer 😅 |
谢谢提醒。关于此问题已经有过非常长久的维护记录为鉴。目前 dio 也有外部的维护者,但最后都因为没有办法受到开源社区的激励而逐渐放弃参与仓库的维护,所以几乎所有的维护者包括原作者都已经失去了维护这个项目的源动力。虽然有我们可以参照的已经失败的分叉,但作为社区的一份子,我们仍然想尝试将像这样的热门 package 维护好,让社区不要总是被背刺。 更加详细的初衷我们会在不久后单独说明,若你能理解那便是我们的荣幸。谢谢! |
Btw, +1 from unping.com to solve this :) would be great to get that into the main branch |
@AlexV525 which "community-maintained dio" fork are you referring to? |
We'll remind related issues soon as we make the fork public. |
Hi everyone! We've made our repo public and published a new version of |
fix 'Future already completed' error when connectTimeout was set.
New Pull Request Checklist
develop
to avoid conflicts (via merge from master or rebase)This merge request fixes / refers to the following issues:
Pull Request Description
Use
Timer
instead ofFuture.delay
to implement connectTimeout on Web.