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

feat(REST): append additional information to the required User Agent #6112

Merged
merged 6 commits into from Jul 16, 2021

Conversation

QSmally
Copy link
Contributor

@QSmally QSmally commented Jul 13, 2021

Please describe the changes this PR makes and why it should be merged:

This PR allows you to append information about the bot (like a website, version) to the end of the (already required) user agent. As specified in the API documentations, users should be able to do so and are encouraged to do it.

Discord.js users could alternatively modify the constants file, where the user agent string is defined, but it can lead to them not following the API specifications.

After having a talk with a few people, I decided to implement a userAgentSuffix option to ClientOptions for people to do this. Types are already updated. An example of the usage is down below.

const { Client } = require("discord.js");
const botClient = new Client({
    userAgentSuffix: [`MyBot/${/* botversion */}`]
});

The implementation of this PR would append the array, joined by , , to the end of the already-default user agent.

  • Default: DiscordBot ($djsurl, $djsversion) Node.js/$nodeversion
  • With suffix: DiscordBot ($djsurl, $djsversion) Node.js/$nodeversion, MyBot/$botversion

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

@QSmally QSmally changed the title Append addition information to the required User Agent Append additional information to the required User Agent Jul 13, 2021
@QSmally QSmally changed the title Append additional information to the required User Agent feat(REST): append additional information to the required User Agent Jul 13, 2021
@DTrombett
Copy link
Contributor

Why not making it a string instead of an array?

@QSmally
Copy link
Contributor Author

QSmally commented Jul 13, 2021

Why not making it a string instead of an array?

It takes the approach of RFC7231. Each segment is divided, and it would be useful if those are reflected in the client option as well.

@DTrombett
Copy link
Contributor

I think the validateOptions method should be updated as well

_validateOptions(options = this.options) {

src/rest/APIRequest.js Outdated Show resolved Hide resolved
@iCrawl iCrawl added this to the Version 13 milestone Jul 14, 2021
Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

Optional destructuring, but otherwise LGTM

src/rest/APIRequest.js Outdated Show resolved Hide resolved
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

I'm not a fan of this code, since it's the checks and user agent are done in every request rather than doing it only once (maybe in the RESTManager?).

I won't block, we can seek an alternative with a smaller performance impact later in another PR.

@iCrawl iCrawl merged commit f200f14 into discordjs:master Jul 16, 2021
@QSmally
Copy link
Contributor Author

QSmally commented Jul 16, 2021

@kyranet - I did not notice that the APIRequest was created on every request. Of course, now it's quite obvious and it was my mistake for not looking into it more. We can look into quite possibly creating the final user agent at startup of the bot, rather than on every request.

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

8 participants