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(ClientOptions): waitGuildTimeout amount client option #6576

Merged
merged 43 commits into from Dec 24, 2021

Conversation

kylerchin
Copy link
Contributor

@kylerchin kylerchin commented Sep 1, 2021

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

Sometimes bots hang while connecting for 15 seconds because 1 single guild is messed up. For bots with Continuous integration, this can create significant impacts to uptime, user experience, and lag.

This adds the optional fetchGuildTimeout option under ClientOptions, where the bot developer can optionally specify how long the client should hang for. If not specified, this will remain the default 15 seconds.

I'm happy to discuss how this can be improved, including the names of the options and variables, and I'd also be happy to write documentation!

👉🏻👈🏻 my first pr to this repo, I'm honoured I can be a part of this community.

EDIT: I'm wondering if the "in X sec" should be changed to "in X ms".

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
  • This PR changes the library's interface (methods or parameters added)

@kylerchin
Copy link
Contributor Author

kylerchin commented Sep 1, 2021

P.S. I've added this to my own bot, cutting the startup time in half in 1 line of code
Before
Before
image
After 3000ms setting, exact reduction of 12 sec
After

Copy link
Contributor

@DTrombett DTrombett left a comment

Choose a reason for hiding this comment

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

This is also missing documentation and validation in the Client class.

src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
src/client/websocket/WebSocketShard.js Outdated Show resolved Hide resolved
@manuelvleeuwen
Copy link
Contributor

Wouldn't shardReadyTimeout be a more appropriate name for this?

@kylerchin
Copy link
Contributor Author

I've tried my best to make the recommended changes, let me know if we need to change anything else or we have any other thoughts about this before merging.

@ImRodry
Copy link
Contributor

ImRodry commented Sep 1, 2021

Yeah I’m pretty sure that text you added goes over the 120 char limit, you should install ESLint to be aware of these things since the CI requires approval to run

kylerchin and others added 2 commits September 1, 2021 13:36
Co-authored-by: Hackerboi 69 <62872992+thehackerboi69github@users.noreply.github.com>
@DTrombett
Copy link
Contributor

DTrombett commented Sep 1, 2021

You can add the default value for this option in the Options.createDefault() static method:

static createDefault() {

And add validation in the Client#_validateOptions() method:
_validateOptions(options = this.options) {

@kylerchin
Copy link
Contributor Author

I'm debating if this should be titled "fetchGuildTimeout" or "fetchGuildsTimeout" or something else. I'm happy to discuss this and improve the experience for developers.

Co-authored-by: monbrey <rsm999@uowmail.edu.au>
src/util/Options.js Outdated Show resolved Hide resolved
kylerchin and others added 2 commits September 2, 2021 15:13
Co-authored-by: Hackerboi 69 <62872992+thehackerboi69github@users.noreply.github.com>
@iCrawl iCrawl modified the milestones: Version 13.4, Version 13.x Nov 11, 2021
@kylerchin
Copy link
Contributor Author

everything in here looks done!

let me know if anything else needs to be changed, otherwise, this will be it! happy that this will get merged soon!

@manuelvleeuwen
Copy link
Contributor

The default value is still not in Options.createDefault()

@Jiralite
Copy link
Member

Check the reviews I submitted also.

@manuelvleeuwen
Copy link
Contributor

What does.that last commit have to do with this PR?

@kylerchin
Copy link
Contributor Author

kylerchin commented Nov 28, 2021 via email

src/structures/BaseGuild.js Outdated Show resolved Hide resolved
@kylerchin
Copy link
Contributor Author

I reverted the changes. Really sorry.

@kylerchin
Copy link
Contributor Author

Those changes have been moved to #7050

@manuelvleeuwen
Copy link
Contributor

The default value is still not in Options.createDefault()

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.

The default is still not added in Options.createDefault()

@kylerchin
Copy link
Contributor Author

Apologies for not updating this, I will change it.

Sorry, i've been busy with finals.

@kylerchin
Copy link
Contributor Author

Again, i'm really sorry about the delay. I added it to options now

@vladfrangu
Copy link
Member

All I want for Christmas..issss... a rebaseeeeee 🎵

@iCrawl iCrawl changed the title Creates the waitGuildTimeout amount client option feat(ClientOptions): waitGuildTimeout amount client option Dec 24, 2021
@iCrawl iCrawl merged commit 2bfc638 into discordjs:main Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet