Skip to content
This repository has been archived by the owner on Jun 27, 2022. It is now read-only.

LL-8941 Conditionally auto disconnect the node-hid-singleton #797

Merged
merged 2 commits into from
Mar 4, 2022
Merged

Conversation

juan-cortes
Copy link
Contributor

Here's the second attempt at getting a automatic release of the node-hid-singleton. After the talk we had the other day @gre I think that by exposing this method setAllowAutoDisconnect at the transport level and allowing it to override the auto-disconnect (basically re-scheduling it again) will allow us to call transport.setAllowAutoDisconnect(false) at the begining of the withDevice implementation on hw/deviceAccess from live-common and unsetting the flag on the onClose for the transport, we essentially bypass the problem we have of long running tasks inside a withDevice instance.

What do you think?

@juan-cortes juan-cortes requested a review from gre February 8, 2022 20:35
@juan-cortes juan-cortes requested review from a team as code owners February 8, 2022 20:35
@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #797 (32b4a72) into master (daf99ca) will increase coverage by 0.49%.
The diff coverage is n/a.

❗ Current head 32b4a72 differs from pull request most recent head e0189b9. Consider uploading reports for the commit e0189b9 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #797      +/-   ##
==========================================
+ Coverage   45.09%   45.59%   +0.49%     
==========================================
  Files          83       82       -1     
  Lines        5060     5005      -55     
  Branches      897      891       -6     
==========================================
  Hits         2282     2282              
+ Misses       2576     2527      -49     
+ Partials      202      196       -6     
Impacted Files Coverage Δ
...ansport-node-hid-singleton/src/TransportNodeHid.ts

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 daf99ca...e0189b9. Read the comment docs.

Copy link
Contributor

@gre gre left a comment

Choose a reason for hiding this comment

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

idea lgtm, some feedback

@juan-cortes
Copy link
Contributor Author

I would like a tagged version of the node-hid-singleton package so that I can make a PR on live-common that disables auto-disconnect inside withDevice and then test it on LLD. Think we can release this to test @gre ? I'd rather not do it considering the mess I caused last time 🙈

@gre
Copy link
Contributor

gre commented Feb 21, 2022

  • can you regen doc?
  • i'll soon issue a new ledgerjs bump from master, after i do can you also merge in the latest master?
  • then yeah i can do a custom release (which is documented in README)

@juan-cortes
Copy link
Contributor Author

I think this is good to merge again @gre ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants