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

Remove dependency on long, abortable-promise-cache #128

Merged
merged 3 commits into from
Aug 30, 2024
Merged

Conversation

cmdcolin
Copy link
Contributor

This removes some dependencies on some packages

The removal of abortable-promise-cache could change aborting behavior in a slight way: if multiple clients try to parseIndex, and one aborts, then all will abort

I don't believe this will affect jbrowse code, and would probably not affect other consumers

This also removes the long package. My testing seemed to conclude that type 'I' in the data records are Uint32 instead of Uint64 requiring the Long package. I couldn't find exact reference to this in the CRAMv3.1 pdf but console logging the buffer showed only 4 bytes instead of 8, and that also corresponds to type i/I in bam-js being 4 byte ints (https://github.com/GMOD/bam-js/blob/507c7284749603862ef72a473db6435c8a865102/src/record.ts#L211-L220)

The typescript in this zone of the code is slightly improved also, but may benefit from some further work (there is technically some usage of Uint8Array|number but it ignores the number usage right now)

@cmdcolin cmdcolin force-pushed the remove_long branch 2 times, most recently from 55e5402 to 9cad54f Compare July 30, 2023 08:58
@codecov
Copy link

codecov bot commented Jul 30, 2023

Codecov Report

Attention: Patch coverage is 96.55172% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 87.36%. Comparing base (8db17b9) to head (9cad54f).

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

Files Patch % Lines
src/craiIndex.ts 95.12% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
- Coverage   90.26%   87.36%   -2.91%     
==========================================
  Files          38       40       +2     
  Lines        1952     2018      +66     
  Branches      400      403       +3     
==========================================
+ Hits         1762     1763       +1     
- Misses        176      249      +73     
+ Partials       14        6       -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rbuels
Copy link
Contributor

rbuels commented Jul 31, 2023

This is fine, though I'm a little worried about the aborting behavior. In JBrowse, if you opened a track, and the .crai download was being slow for whatever reason, and the user scrolls around a bit, I think this could well lead to the track just breaking and needing to be reloaded.

@cmdcolin cmdcolin force-pushed the remove_long branch 2 times, most recently from 0ca8065 to f22e073 Compare March 18, 2024 14:55
@cmdcolin cmdcolin force-pushed the remove_long branch 3 times, most recently from a95bcf3 to cc521d4 Compare August 30, 2024 18:49
@cmdcolin cmdcolin merged commit 61791ba into master Aug 30, 2024
1 check passed
@cmdcolin cmdcolin deleted the remove_long branch August 30, 2024 18:57
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