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: retry on abort and limit retry count to 10 #655

Merged
merged 10 commits into from
May 28, 2019
Merged

Conversation

thebrianchen
Copy link

@thebrianchen thebrianchen commented May 23, 2019

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 23, 2019
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

This looks good and solves the problem. I do have a suggestion to simplify it a little bit, and some nits. Thanks Mr Chen!

dev/src/backoff.ts Outdated Show resolved Hide resolved
dev/src/watch.ts Outdated Show resolved Hide resolved
dev/src/watch.ts Outdated Show resolved Hide resolved
dev/src/watch.ts Outdated Show resolved Hide resolved
dev/src/watch.ts Outdated Show resolved Hide resolved
dev/test/watch.ts Outdated Show resolved Hide resolved
@thebrianchen thebrianchen removed their assignment May 24, 2019
@thebrianchen
Copy link
Author

Thanks for the review mrschmidt!

Copy link
Contributor

@schmidt-sebastian schmidt-sebastian left a comment

Choose a reason for hiding this comment

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

95% LGTM!

Please note that this repo now also uses https://www.conventionalcommits.org/

This means that your commit message/PR title drives not only the Changelog but is also used to determine the version of the next release. I would consider this PR a "fix". If you pre-face your commit message with fix, our GitHub tooling will automatically create a Patch release once this is merged (*)

(*) Note that there is already a feat: PR that was merged, and as such the patch release will actually be a minor release.

dev/src/backoff.ts Show resolved Hide resolved
dev/src/backoff.ts Outdated Show resolved Hide resolved
dev/src/watch.ts Outdated Show resolved Hide resolved
@thebrianchen thebrianchen changed the title Retry on abort and limit retry count to 10 fix: retry on abort and limit retry count to 10 May 24, 2019
@codecov
Copy link

codecov bot commented May 24, 2019

Codecov Report

Merging #655 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #655      +/-   ##
==========================================
+ Coverage   61.55%   61.63%   +0.07%     
==========================================
  Files          21       21              
  Lines        3418     3425       +7     
  Branches      459      460       +1     
==========================================
+ Hits         2104     2111       +7     
  Misses       1252     1252              
  Partials       62       62
Impacted Files Coverage Δ
dev/src/backoff.ts 100% <100%> (ø) ⬆️
dev/src/watch.ts 97.68% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaf5a4e...74641df. Read the comment docs.

@codecov
Copy link

codecov bot commented May 24, 2019

Codecov Report

Merging #655 into master will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #655      +/-   ##
==========================================
+ Coverage   61.46%   61.64%   +0.17%     
==========================================
  Files          21       21              
  Lines        3418     3426       +8     
  Branches      459      460       +1     
==========================================
+ Hits         2101     2112      +11     
+ Misses       1254     1252       -2     
+ Partials       63       62       -1
Impacted Files Coverage Δ
dev/src/backoff.ts 100% <100%> (ø) ⬆️
dev/src/watch.ts 97.69% <100%> (+1.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eaf5a4e...8c1f6cf. Read the comment docs.

@schmidt-sebastian
Copy link
Contributor

I pushed a small cleanup to this branch to remove the duplicate .catch() clause. Let me know if you disagree otherwise I will merge this tomorrow. Thanks!

@schmidt-sebastian schmidt-sebastian merged commit 9e97656 into master May 28, 2019
@schmidt-sebastian schmidt-sebastian deleted the bc/abort branch May 30, 2019 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onSnapshot listener dies after some time with "Error 10: ABORTED"
3 participants