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

client.get does not take the same "torrentId" that client.add does #2222

Closed
mitra42 opened this issue Nov 14, 2021 · 6 comments
Closed

client.get does not take the same "torrentId" that client.add does #2222

mitra42 opened this issue Nov 14, 2021 · 6 comments
Labels

Comments

@mitra42
Copy link

mitra42 commented Nov 14, 2021

What version of this package are you using?
1.5.6

What operating system, Node.js, and npm version?
OSX node 14.7.2 npm 7.24.1

What happened?
I did a client.add('http://localhost:4250/video/uptake blip video.torrent') which worked fine since the URL of the torrent is a valid torrentId for the "add" function

However client.get('http://localhost:4250/video/uptake blip video.torrent') returns null, even after the add has called its callback.

client.get(infoHash) works

STR
const torrentId = 'http://localhost:4250/video/uptake blip video.torrent'; # I'm using a local server for testing
WTclient.add(torrentId, function (torrent) {
console.log('T1=',WTclient.get(torrentId)) # null
console.log('T2=',WTclient.get(torrent.infoHash)) # Torrent
});

What did you expect to happen?
I would have expected that a valid torrentId passed to add, would also work for get

Are you willing to submit a pull request to fix this bug?
I'm not sure how to fix this while keeping with the design philosophy, the key challenge is that the "torrentId" used for client.add does not seem to be stored on either the torrent or the client.
I think .... that keeping a object on the client that hashes from torrentId to torrent would work, the key could be anything passed to torrent.add, so it would be a quick lookup,

Note - I can work around this - now I've realized why its not working, its not urgent for me, but if this is desired behavior, or not going to be fixed quickly, then I'd suggest change the API doc to make it clear that "torrentId" when passed to client.get is different from torrentId passed to client.add

@github-actions
Copy link

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

@github-actions github-actions bot added the stale label Jan 13, 2022
@mitra42
Copy link
Author

mitra42 commented Jan 13, 2022

Yes - its still relevant, I'm not sure if its an API bug or at the very least a documentation bug. AFAIK noon familiar with the project has looked at it, so that's the blockr.

@DiegoRBaquero
Copy link
Member

When creating a torrent with a link (in this case), parse torrent uses the remote method: https://github.com/webtorrent/webtorrent/blob/master/lib/torrent.js#L218

But this is not the case when using Client.get, it does not support remote torrents. The functionality could be added so that remote torrents are also parsed when passed to Client.get.

Happy to review a PR if you are up for it

@mitra42
Copy link
Author

mitra42 commented Jan 13, 2022

I've found the codebase hard to get my head around, especially since functions aren't commented so its hard to know what they are intended to do, and therefore how to fix them without breaking something.

I've took a but the solution to me seems different ....
In client.add at

this.torrents.push(torrent)
it does this.torrents.push(torrent)

In client.get at

if (this.torrents.includes(torrentId)) return torrentId
it does (this.torrents.includes(torrentId))

I don't think client.get should do an asynchronous https request for the torrent at this point (as is done in parse torrent), especially as this would make client.get asynchronous and complicate all kinds of client code. Instead, I think either:

  • a: the URL passed to parseTorrent.remote at

    parseTorrent.remote(torrentId, (err, parsedTorrent) => {
    should either be stored in the returned torrent, OR

  • b: a seperate Object created which client.get could look up to see if something has previously been passed to client.add, which is non-trivial since that would need to be done at that same point in _onTorrentId (which is when you know its remote), but the object belongs at the client level, which is why think "a" would be better.

The use case by the way is simple. I have a simple WebComponent that is a WebTorrentVideo that is passed a torrent URL and file name, and displays the video. (it has been added to a more comprehensive WebComponent that already displays other kinds of video, such as YouTube)

client.add throws an error if passed a duplicate (which in itself I think is a design flaw), so my first design was to do a client.get and skip the add if the torrent has already being loaded, but that fails on URLs so ...

Now what I do is within my WebComponent keep a mapping so the code effectively looks like ...

if (!WTclient) { WTclient = new WebTorrent(); WTclient.torrentsAdded = [] } // Create empty array in client
...
if ( !WTclient.get(torrentId) && !WTclient.torrentsAdded.includes(torrentId))) {
   WTclient.torrentsAdded.push(torrentId)
   WTclient.add(torrentId, function (torrent) { ... }
}

@github-actions github-actions bot removed the stale label Jan 14, 2022
@github-actions
Copy link

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

@mitra42
Copy link
Author

mitra42 commented Mar 23, 2022

Why was this closed - it looks like it was closed by a bot, which suggests there are a bunch of other issues reported closed without being fixed ?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants