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

refactor(Sharding): Update Shard#broadcastEval to v13 syntax. #695

Merged
merged 15 commits into from Jul 24, 2021

Conversation

WiseDevHelper
Copy link
Contributor

@WiseDevHelper WiseDevHelper commented Jul 7, 2021

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

Demo

This PR updates the Sharding section of the guide to be up-to-date with the breaking change ShardingManager#broadcastEval received in discordjs/discord.js#5756. This change was made to fix an RCE vulnerability explained in this repo.

A lot of things might need to be adjusted/fixed/rewritten, and I will happy to do so when suggested.

Status

  • Update Sharding.
    • Main page.
    • Addtional Information.
    • Extended Changes.
  • Add info about context.
  • Update code samples.
    • Getting Started.
    • Extended Changes.

@WiseDevHelper WiseDevHelper marked this pull request as ready for review July 8, 2021 05:45
Copy link
Member

@Danktuary Danktuary left a comment

Choose a reason for hiding this comment

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

A few nitpicks, looks good otherwise 👍🏼

guide/sharding/additional-information.md Outdated Show resolved Hide resolved
guide/sharding/additional-information.md Outdated Show resolved Hide resolved
guide/sharding/additional-information.md Outdated Show resolved Hide resolved
guide/sharding/additional-information.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Outdated Show resolved Hide resolved
Co-authored by: Danktuary <sanc.pw@gmail.com>
@WiseDevHelper
Copy link
Contributor Author

Applied! 👍

For some reason, there is no spacing between the heading and content here:
image

How can we fix this?

@Danktuary
Copy link
Member

Oh, that's strange. Looks fine at a glance, but maybe it's because <DocsLink> is used for the first word. Try restructuring the sentence to One of the most common sharding utility methods you'll be using is <DocsLink path="class/ShardClientUtil?scrollTo=fetchClientValues">Shard#fetchClientValues</DocsLink>. Not sure why that would cause that though.

If that doesn't work, just add a <br /> before the paragraph.

@WiseDevHelper
Copy link
Contributor Author

Oh, that's strange. Looks fine at a glance, but maybe it's because <DocsLink> is used for the first word. Try restructuring the sentence to One of the most common sharding utility methods you'll be using is <DocsLink path="class/ShardClientUtil?scrollTo=fetchClientValues">Shard#fetchClientValues</DocsLink>. Not sure why that would cause that though.

This worked! 👍

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.

Please don't just remove quotes, that's not how you translate it.

guide/sharding/README.md Outdated Show resolved Hide resolved
guide/sharding/README.md Outdated Show resolved Hide resolved
guide/sharding/README.md Outdated Show resolved Hide resolved
guide/sharding/README.md Outdated Show resolved Hide resolved
guide/sharding/additional-information.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Outdated Show resolved Hide resolved
guide/sharding/additional-information.md Outdated Show resolved Hide resolved
guide/sharding/additional-information.md Outdated Show resolved Hide resolved
guide/sharding/additional-information.md Outdated Show resolved Hide resolved
guide/sharding/additional-information.md Outdated Show resolved Hide resolved
guide/sharding/additional-information.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Show resolved Hide resolved
Copy link
Member

@almostSouji almostSouji left a comment

Choose a reason for hiding this comment

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

Also needs to address code samples for sharding (remove v11/v12 folders)

Copy link
Member

@Danktuary Danktuary left a comment

Choose a reason for hiding this comment

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

Replacing the .broadcastEval(client => ...) with .broadcastEval(c => ...), because client is already declared in the upper scope.

code-samples/sharding/getting-started/13/bot.js Outdated Show resolved Hide resolved
code-samples/sharding/getting-started/13/bot.js Outdated Show resolved Hide resolved
code-samples/sharding/extended/13/bot.js Outdated Show resolved Hide resolved
code-samples/sharding/extended/13/bot.js Outdated Show resolved Hide resolved
guide/sharding/README.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Outdated Show resolved Hide resolved
guide/sharding/additional-information.md Outdated Show resolved Hide resolved
guide/sharding/extended.md Outdated Show resolved Hide resolved
Co-authored-by: Sanctuary <Danktuary@users.noreply.github.com>
@Danktuary Danktuary merged commit 5063ab3 into discordjs:v13-prep Jul 24, 2021
@WiseDevHelper WiseDevHelper deleted the v13-broadcast-eval branch July 25, 2021 06:45
@WiseDevHelper WiseDevHelper mentioned this pull request Jul 25, 2021
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants