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

Provide a way to give extra data on announce #107

Merged
merged 1 commit into from
Feb 29, 2016

Conversation

yciabaud
Copy link
Contributor

@yciabaud yciabaud commented Feb 3, 2016

This PR adds a callback named getAnnounceOpts to the client options to be able to send updated data to the tracker (downloaded, uploaded, left and extra...).

The extra data can be handled in a tracker by listening to the event params.

I will send PRs for torrent-discovery and webtorrent in order to make this available in webtorrent.

@yciabaud yciabaud changed the title Provide a way to give extra data announce Provide a way to give extra data on announce Feb 3, 2016
@feross
Copy link
Member

feross commented Feb 8, 2016

Nice! Thanks for coming with a pretty clean way to finally fix #3 (which has been open for almost 2 years!!)

My only concern is forcing the use of "extra" for the name of the http/websocket tracker parameter. Is this part of a BEP or some standard? If not, I'd rather just let the user specify any parameter names that they want. Just add the params the user specifies to the params dictionary using the xtend package (already in package.json)

Also, this needs documentation in the readme.


if (opts.getAnnounceOpts == null) {
opts.getAnnounceOpts = self._getAnnounceOpts
}
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary. All tracker types already have a reference to the Client instance. So they should check for self.client._getAnnounceOpts directly.

@yciabaud
Copy link
Contributor Author

yciabaud commented Feb 8, 2016

You're right, I was not comfortable with the use of extra neither but I didn't think about extending params. I am changing this.

@yciabaud
Copy link
Contributor Author

yciabaud commented Feb 8, 2016

Actually the getAnnounceOpts function looks like :

getAnnounceOpts: function () {
  return {
    uploaded: self.uploaded,
    downloaded: self.downloaded,
    left: self.length - self.downloaded,
    extraAnnounceOpts: {
      myOwnKey: myOwnValue,
      myOtherKey: myOtherValue
    }
  }
}

Do you believe I should flatten this as we are planning to mix it in the request?

@feross
Copy link
Member

feross commented Feb 29, 2016

@yciabaud Yes, let's make it flattened everywhere.

@feross
Copy link
Member

feross commented Feb 29, 2016

I'm now working on getting this cleaned up and merged.

feross added a commit that referenced this pull request Feb 29, 2016
Provide a way to give extra data on announce
@feross feross merged commit 1db2cb8 into webtorrent:master Feb 29, 2016
@yciabaud
Copy link
Contributor Author

OK sorry for the delay, I did work on the documentation and on flattening all this but I did not manage to test it thoroughly.

Do you want me to push these improvements in another PR?

@feross
Copy link
Member

feross commented Feb 29, 2016

I've already fixed it up to flatten. Will push the changes here soon. Thanks for offering!

feross added a commit that referenced this pull request Mar 1, 2016
feross added a commit that referenced this pull request Mar 1, 2016
feross added a commit that referenced this pull request Mar 1, 2016
@feross
Copy link
Member

feross commented Mar 1, 2016

7.3.0.

@feross feross mentioned this pull request Mar 1, 2016
@yciabaud
Copy link
Contributor Author

yciabaud commented Mar 1, 2016

Ok thats great, its close to what I wrote on my side. I can add a documentation in the readme if you need.

@feross
Copy link
Member

feross commented Mar 1, 2016

Sure, take a look at what's already there. Feel free to add to it :)

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.

None yet

2 participants