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(Activity): add missing fields #4984

Merged
merged 8 commits into from Apr 6, 2021

Conversation

advaith1
Copy link
Contributor

@advaith1 advaith1 commented Nov 7, 2020

Please describe the changes this PR makes and why it should be merged:
Adds id, platform, syncID, sessionID, and buttons to the Activity object (discord/discord-api-docs#2219)

wasn't sure if I should do something for ClientPresence because bots can't send these, but it seems to already have code to handle many properties that bots can't send [msg]

Status

  • 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

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.

typings/index.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Jan <66554238+Vaporox@users.noreply.github.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.

Waiting for the linked PR to land first, but if there are no further changes, LGTM.

@advaith1 advaith1 changed the title feat(Activity): add id and platform feat(Activity): add missing fields Nov 11, 2020
typings/index.d.ts Outdated Show resolved Hide resolved
src/structures/Presence.js Outdated Show resolved Hide resolved
src/structures/Presence.js Outdated Show resolved Hide resolved
src/structures/Presence.js Outdated Show resolved Hide resolved
@advaith1
Copy link
Contributor Author

@Vaporox all the other ones use ||, why make these different? probably won't actually make a difference here

@kyranet
Copy link
Member

kyranet commented Dec 23, 2020

@Vaporox all the other ones use ||, why make these different? probably won't actually make a difference here

Because we landed requirement for Node.js v14 (which implements ??) fairly recently, and we haven't refactored all the eligible ||s yet.

@advaith1
Copy link
Contributor Author

So will we use ?? as the default going forward?

@kyranet
Copy link
Member

kyranet commented Dec 23, 2020

So will we use ?? as the default going forward?

That's correct.

Co-authored-by: Jan <66554238+Vaporox@users.noreply.github.com>
@advaith1
Copy link
Contributor Author

advaith1 commented Dec 23, 2020

oh looks like syncID was already there but undocumented (in djs and discord) and doesn't default to null 🤔 should I remove the old one?
https://github.com/discordjs/discord.js/pull/4984/files#diff-72d7646d708e843ab070501124748d67961184ff3497db3f56dd5e6ac1f2ed92R237

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.

Good to go, once the upstream PR is merged.

@vaporoxx vaporoxx mentioned this pull request Apr 6, 2021
@iCrawl iCrawl merged commit 63ff6a0 into discordjs:master Apr 6, 2021
@advaith1 advaith1 deleted the activity-id-platform branch April 6, 2021 22:45
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

6 participants