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: switch api and gateway to V8 #4879

Merged
merged 46 commits into from
Feb 11, 2021
Merged

Conversation

NotSugden
Copy link
Contributor

@NotSugden NotSugden commented Oct 2, 2020

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

V8 diff - discord/discord-api-docs@545ff4a

Changes currently in this PR:

  • A couple minor documentation fixes
  • Bumped Gateway and REST versions to 8
  • Modifies BitField to support BigInts and Permissions to use them
  • Stops the channelCreate event emitting for DM Channels as that behaviour seems to have been removed
    • channelDelete still emits, so that's been left as-is
  • Roles are no longer used to update a GuildMember with the Presence Update event as the roles are no longer sent
  • Fixed an error that would show double stack trace
  • Updated presence updating methods to use only the data available for a Presence Update
    • Along with this said functions are no longer async
  • Documented ClientUser#edit
  • Removed Guild#embed* in favour of Guild#widget*
  • Fixed a couple documentation typos
  • Added new OverwriteTypes constant to match API Change
  • Changed a few methods to be async
  • Removed ClientOptions#ws#intents and replaced it with ClientOptions#intents, and made it required as the V8 Gateway requires them
  • Documented new error codes
  • Removed options from Guild#fetchIntegrations() as the query parameter was removed in V8 (Add v8 integration changes to changelog discord/discord-api-docs#2238)
    • also made the function async
  • Made GuildChannel#updateOverwrite() async
  • Made PermissionOverwrites#update() async
  • Made PermissionOverwrites#delete() async
  • Updated tests to use non-privileged intents
Notes about the non-usage of BigInt literals The usage of BigInt Litertals is due to a bug with the library handling the jsdoc, failing the CI

Status

  • Code changes have been tested against the Discord API, or there are no code changes
    • I haven't experienced any 'broken' behaviour, but if anyone would like to double check thats fine by me
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@NotSugden NotSugden changed the title refactor: use BigInts for Permissions refactor: switch api and gateway to V8 Oct 2, 2020
@NotSugden NotSugden marked this pull request as draft October 2, 2020 22:20
@NotSugden NotSugden marked this pull request as ready for review October 3, 2020 02:00
@NotSugden NotSugden marked this pull request as draft October 3, 2020 02:33
src/structures/GuildAuditLogs.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/GuildChannel.js Outdated Show resolved Hide resolved
src/structures/GuildChannel.js Outdated Show resolved Hide resolved
src/structures/Role.js Outdated Show resolved Hide resolved
src/util/BitField.js Outdated Show resolved Hide resolved
src/util/Permissions.js Outdated Show resolved Hide resolved
src/util/Permissions.js Outdated Show resolved Hide resolved
src/util/Permissions.js Show resolved Hide resolved
src/structures/GuildAuditLogs.js Outdated Show resolved Hide resolved
src/structures/ClientPresence.js Outdated Show resolved Hide resolved
src/structures/ClientPresence.js Outdated Show resolved Hide resolved
src/structures/PermissionOverwrites.js Outdated Show resolved Hide resolved
src/util/Constants.js Outdated Show resolved Hide resolved
@advaith1
Copy link
Contributor

advaith1 commented Oct 3, 2020

imo:

  • ClientOptions#intents should work for setting intents, like the other options that are intended to be changed (and are not sent raw)
  • we should not be setting any default intents, bot devs should have to put the intents they need, otherwise, this basically removes the whole point of intents

Copy link
Contributor

@papaia papaia left a comment

Choose a reason for hiding this comment

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

Reflecting the second point in #4879 (comment).

src/util/Constants.js Outdated Show resolved Hide resolved
src/util/Constants.js Outdated Show resolved Hide resolved
@zaida04 zaida04 mentioned this pull request Oct 4, 2020
3 tasks
@NotSugden
Copy link
Contributor Author

imo:

  • ClientOptions#intents should work for setting intents, like the other options that are intended to be changed (and are not sent raw)
  • we should not be setting any default intents, bot devs should have to put the intents they need, otherwise, this basically removes the whole point of intents

sorry that took so long, but resolved in 9db4acc

src/structures/Role.js Outdated Show resolved Hide resolved
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.

This PR will need extensive testing before merging 🙏

Some nitpicks for now

src/structures/GuildAuditLogs.js Outdated Show resolved Hide resolved
src/structures/GuildChannel.js Outdated Show resolved Hide resolved
src/structures/PermissionOverwrites.js Outdated Show resolved Hide resolved
src/util/Permissions.js Outdated Show resolved Hide resolved
src/structures/ClientUser.js Outdated Show resolved Hide resolved
@NotSugden NotSugden marked this pull request as ready for review October 23, 2020 02:57
@NotSugden NotSugden marked this pull request as draft October 23, 2020 02:57
@NotSugden NotSugden marked this pull request as ready for review November 14, 2020 04:16
@NotSugden
Copy link
Contributor Author

ok! i've done a quick test and everything seems to be working fine now, if anyone else wants to confirm that feel free, marking as ready for review ig

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.

This PR is not ready. There are REST changes that need to be done to account for the ratelimits finally respecting specs...

src/structures/ClientUser.js Outdated Show resolved Hide resolved
@iCrawl
Copy link
Member

iCrawl commented Dec 14, 2020

@NotSugden this needs a rebase

@NotSugden
Copy link
Contributor Author

@iCrawl done

Copy link
Contributor

@vaporoxx vaporoxx left a comment

Choose a reason for hiding this comment

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

next to that, should options.intents really be required? (i.e. WebhookClients, i don't know if those actually use intents)

typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
@NotSugden
Copy link
Contributor Author

next to that, should options.intents really be required? (i.e. WebhookClients, i don't know if those actually use intents)

i have made a new type WebhookClientOptions (which is just a Pick of all applicable options from ClientOptions)

Copy link
Contributor

@Fyko Fyko left a comment

Choose a reason for hiding this comment

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

interface PermissionFlags extends Record<PermissionString, number> {}

needs to be updated to

interface PermissionFlags extends Record<PermissionString, bigint> {}

@NotSugden
Copy link
Contributor Author

rebased & suggested changes applied

src/util/BitField.js Outdated Show resolved Hide resolved
src/managers/RoleManager.js Outdated Show resolved Hide resolved
@NotSugden
Copy link
Contributor Author

fixed the docs error 🎉

@vladfrangu
Copy link
Member

As a note for maintainers, however, please make sure this PR has been tested across everything it modifies before merging 🙏

@typpo
Copy link

typpo commented Feb 6, 2021

I have tested message create, react, edit, and delete rate limits on @NotSugden's branch and had no problems :)

Copy link
Member

@SpaceEEC SpaceEEC left a comment

Choose a reason for hiding this comment

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

🎉

@iCrawl iCrawl merged commit ee5bc1a into discordjs:master Feb 11, 2021
Fyko added a commit to Fyko/discord.js that referenced this pull request Feb 14, 2021
sntran added a commit to vietcode/DataHoarder that referenced this pull request Feb 17, 2021
smicle added a commit to smicle/discord.js that referenced this pull request Feb 18, 2021
@JMTK
Copy link
Contributor

JMTK commented Feb 19, 2021

After this was merged, my bot user is unable to initially receive DMs. If I send myself a message using an eval command from the bot, then it will start receiving messages. Is this expected given the bullet point channelCreate events are no longer emitted for DMs?

I sent myself "hi" from a Guild channel instead client.users.cache.get(message.author.id).send('hi'). After that, I received message events from my user.
image

Should I open an issue ticket?

@advaith1
Copy link
Contributor

you can fix it by enabling the CHANNEL partial; we should make that always on (should have been in this pr)

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.

Switch to _new permission string fields