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

Option to destroy store on torrent removal (delete files from disk) #1364

Open
wants to merge 2 commits into
base: master
from

Conversation

@KayleePop
Copy link
Contributor

KayleePop commented Apr 25, 2018

Intended to replace #1102

Should work with custom stores like indexeddb as well, but I didn't test that.

API is this:

client.remove(torrentId, {destroyStore: true}, () => console.log('files deleted'))

torrent.destroy({destroyStore: true}, () => console.log('files deleted'))

fixes #1000

@KayleePop KayleePop force-pushed the KayleePop:destroy branch from 9a11565 to 07f8dba Apr 25, 2018
@DiegoRBaquero

This comment has been minimized.

Copy link
Member

DiegoRBaquero commented Jul 24, 2018

As opts is optional, if you don't included but you have the callback function, this would break the API. The way to fix it is this: https://github.com/webtorrent/webtorrent/pull/1102/files#diff-168726dbe96b3ce427e7fedce31bb0bcR383

Why would you like to replace #1102?

@KayleePop

This comment has been minimized.

Copy link
Contributor Author

KayleePop commented Jul 26, 2018

Both client.remove() and torrent.destroy() pass both opts and the callback into torrent._destroy before they're used, and I cover that scenario there https://github.com/webtorrent/webtorrent/pull/1364/files#diff-65b0a569f99e647ae33e8c31b9bf72baR647

If the API was broken like that, then these tests would fail https://github.com/webtorrent/webtorrent/blob/master/test/torrent-destroy.js#L19-L22

Should I handle it in all three functions for clarity?

 

Yeah, looking back at #1102 it seems to almost the same as this with minor differences.

I don't remember why I wanted to replace it. I think I didn't look closely enough at the changes and thought it was abandoned. I should've asked there first.

Should I close this? It's no big deal.

The only real differences between the two are that this one tests for both API functions and works with all custom stores. I could submit a review on the other one for that.

@KayleePop KayleePop force-pushed the KayleePop:destroy branch from 07f8dba to e57804c Aug 26, 2018
@KayleePop KayleePop changed the title Option to destroy store on torrent removal (delete files from disk) WIP: Option to destroy store on torrent removal (delete files from disk) Aug 26, 2018
@KayleePop KayleePop force-pushed the KayleePop:destroy branch from e57804c to dc03ee2 Aug 26, 2018
@KayleePop KayleePop changed the title WIP: Option to destroy store on torrent removal (delete files from disk) Option to destroy store on torrent removal (delete files from disk) Aug 26, 2018
@KayleePop KayleePop force-pushed the KayleePop:destroy branch from dc03ee2 to 3e8c53c Aug 27, 2018
@KayleePop KayleePop force-pushed the KayleePop:destroy branch from 3e8c53c to 1ed6013 Oct 1, 2018
@KayleePop KayleePop force-pushed the KayleePop:destroy branch from 1ed6013 to 0f8a0f6 Oct 1, 2018
@guanzo

This comment has been minimized.

Copy link
Contributor

guanzo commented Dec 12, 2018

Any progress on this? I'm using WebTorrent in the browser and would like to delete file data with client.remove()/torrent.destroy()

@KayleePop

This comment has been minimized.

Copy link
Contributor Author

KayleePop commented Dec 12, 2018

You can do this without this PR:

torrent.destroy((err) => {
  if (err) throw err

  torrent.store.destroy((err) => {
    if (err) throw err

    console.log('destroyed and files removed')
  })
})

You might have to do it in the opposite order (destroy its store, then the torrent).

You also only need to delete it if you use a custom chunk store like idbkv-chunk-store. Otherwise, in the browser, the data is never written to the disk and only temporarily stored in memory.

@stale

This comment has been minimized.

Copy link

stale bot commented Mar 12, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Mar 12, 2019
@stale stale bot closed this Mar 20, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2019
@feross feross added the enhancement label Sep 6, 2019
@webtorrent webtorrent unlocked this conversation Sep 6, 2019
@feross feross reopened this Sep 6, 2019
@stale stale bot removed the stale label Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.