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: api v9 and threads #5570

Merged
merged 23 commits into from Jun 24, 2021
Merged

feat: api v9 and threads #5570

merged 23 commits into from Jun 24, 2021

Conversation

ckohen
Copy link
Member

@ckohen ckohen commented Apr 30, 2021

Please describe the changes this PR makes and why it should be merged:
This is a very preliminary attempt at adding threads: discord/discord-api-docs#2855

⚠️ Disclaimer: as the docs are only semi-finalized and still subject to change, this will remain a draft until they are finalized and threads are actually released and can be test. This is primarily to track progress on the implementation and to ensure all efforts to implement threads can be focused in one place.

Notes:

  1. As of now, threads are included in the channel caches for the client and guild alongside in their respective text channels threads cache. This is because threads are channels at heart, and allows access to said thread channels as you would any normal channel.
  2. Although threads are present in the channel cache, there is a separate event structure in the API, and such those events are mirrored as well.
  3. A minor warning about thread channels in the channel cache: usually discord.js expects ALL channels to be cached. With threads, if the client does not have access to the parent channel, then it does not know the thread exists, so the channels can be uncached.

Feel free to make suggestions for major changes in this code, I am open to implementing this any different way.

Status and versioning classification:

  • This PR changes the library's interface (methods or parameters added)
  • I know how to update typings and have done so, or typings don't need updating

@iCrawl
Copy link
Member

iCrawl commented Apr 30, 2021

Can't wait to see this merged in v15

@ckohen
Copy link
Member Author

ckohen commented Apr 30, 2021

Lol yeah, looking forward to that

Copy link
Contributor

@ImRodry ImRodry left a comment

Choose a reason for hiding this comment

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

Just a few minor things I think should be improved on as well as a few questions

src/managers/ThreadManager.js Outdated Show resolved Hide resolved
src/managers/ThreadManager.js Outdated Show resolved Hide resolved
src/managers/UserManager.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/Message.js Outdated Show resolved Hide resolved
src/structures/ThreadChannel.js Outdated Show resolved Hide resolved
src/structures/ThreadChannel.js Outdated Show resolved Hide resolved
src/structures/ThreadChannel.js Outdated Show resolved Hide resolved
src/structures/ThreadChannel.js Outdated Show resolved Hide resolved
src/structures/ThreadChannel.js Outdated Show resolved Hide resolved
@ckohen ckohen force-pushed the threads branch 2 times, most recently from 753301a to 092376f Compare May 26, 2021 02:15
@vladfrangu
Copy link
Member

You wouldn't get this from any other guy, but this needs a rebase <3

src/client/actions/ThreadListSync.js Outdated Show resolved Hide resolved
src/client/actions/ThreadListSync.js Outdated Show resolved Hide resolved
});

/**
* Emitted whenever the client user gains access to a text or news channel
Copy link
Member

Choose a reason for hiding this comment

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

Description isn't accurate

Copy link
Member Author

Choose a reason for hiding this comment

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

What isn't accurate about this?

Copy link
Member

Choose a reason for hiding this comment

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

they're not text or news channels, they're thread channels :P

Copy link
Member Author

Choose a reason for hiding this comment

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

But the event is emitted when you gain access to the parent channel, being a text or news channel, even if there are no threads.

Copy link
Member

Choose a reason for hiding this comment

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

But the event is emitted when you gain access to the parent channel, being a text or news channel, even if there are no threads.

The event is WHAT? So... when you get access to a channel, you now...get..an event...? But bots/users already know about all channels from the moment they connect...

Choose a reason for hiding this comment

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

even if there are no threads.

rly? I think every place that sends it is checking if there are threads, how'd you reproduce it!

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, cannot reproduce with no threads anymore. Although, it does seem weird to not sync threads when there were previously threads. E.g. if you have active threads, lose access to a channel, thread is marked archived, gain access to the channel. Since there is no THREAD_DELETE fired here and we obviously don't get THREAD_UPDATE while not having access to the channel, the cache is now out of date and doesn't get updated (in this case, removing the now archived threads

src/client/actions/ThreadListSync.js Outdated Show resolved Hide resolved
src/client/actions/ThreadListSync.js Outdated Show resolved Hide resolved
src/structures/ThreadChannel.js Outdated Show resolved Hide resolved
src/structures/ThreadChannel.js Outdated Show resolved Hide resolved
src/structures/ThreadMember.js Outdated Show resolved Hide resolved
src/structures/ThreadMember.js Outdated Show resolved Hide resolved
src/structures/ThreadMember.js Outdated Show resolved Hide resolved
src/structures/ThreadChannel.js Outdated Show resolved Hide resolved
src/managers/ChannelManager.js Show resolved Hide resolved
src/structures/ThreadChannel.js Outdated Show resolved Hide resolved
src/structures/ThreadChannel.js Outdated Show resolved Hide resolved
src/util/Constants.js Show resolved Hide resolved
src/structures/ThreadChannel.js Outdated Show resolved Hide resolved
ckohen and others added 18 commits June 24, 2021 01:09
Co-authored-by: Amish Shah <dev@shah.gg>
Co-authored-by: Amish Shah <dev@shah.gg>
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
Co-Authored-By: SynthGhost <60333233+synthghost@users.noreply.github.com>
Co-Authored-By: SpaceEEC <24881032+SpaceEEC@users.noreply.github.com>
Co-authored-by: Elliot <elliot@maisl.fr>
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
Co-Authored-By: SpaceEEC <24881032+SpaceEEC@users.noreply.github.com>
Co-Authored-By: Antonio Román <kyradiscord@gmail.com>
Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com>
Co-authored-by: Vlad Frangu <kingdgrizzle@gmail.com>
Co-authored-by: Antonio Román <kyradiscord@gmail.com>
@ckohen
Copy link
Member Author

ckohen commented Jun 24, 2021

Rebased and added the new thing from api docs pr

@iCrawl iCrawl merged commit 7346621 into discordjs:master Jun 24, 2021
@ajpalkovic
Copy link

Woohoo, congrats!
Thanks y'all :)

@ckohen ckohen deleted the threads branch June 24, 2021 19:53
iCrawl pushed a commit that referenced this pull request Jun 24, 2021
@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
@itisdoc
Copy link

itisdoc commented Jun 29, 2021

Lets go! Now I dont have to use private channels for my ticketing system

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.

Add support for API v9 and Threads