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

Add abortable promise cache for recovering from index loading error #74

Merged
merged 5 commits into from
Jul 2, 2020

Conversation

cmdcolin
Copy link
Contributor

@cmdcolin cmdcolin commented Jul 1, 2020

The code in the last PR #73 went some ways towards enabling reload, however it was not able to recover from a 404 on the index file

This makes it so that the index is fetched from the abortable-promise-cache, basically an abortable-memoize. Goes some ways towards #27 and IMO is fairly easy to reason about

Con: added dependencies, but hopefully not too bad

Goes to the larger goal of enabling better recovery from errors

@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

Merging #74 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
+ Coverage   89.68%   89.70%   +0.02%     
==========================================
  Files          34       34              
  Lines        1512     1515       +3     
==========================================
+ Hits         1356     1359       +3     
  Misses        156      156              
Impacted Files Coverage Δ
src/craiIndex.js 96.42% <100.00%> (+0.13%) ⬆️

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 5fbc29b...102acb6. Read the comment docs.

@cmdcolin
Copy link
Contributor Author

cmdcolin commented Jul 1, 2020

In case it helps, the test from #73 only performed a test for the CRAM file itself recovering, so this tests for the CRAI in this case (didn't try other index formats)

@cmdcolin
Copy link
Contributor Author

cmdcolin commented Jul 1, 2020

There is a test for this behavior now that fails on master but succeeds here

@cmdcolin cmdcolin requested a review from rbuels July 1, 2020 21:20
@rbuels rbuels merged commit 8fe53eb into master Jul 2, 2020
@rbuels rbuels deleted the add_abortable_promise_cache branch July 2, 2020 17:27
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.

2 participants