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(WelcomeScreen): welcome screens #5490

Merged
merged 45 commits into from Jun 19, 2021

Conversation

almostSouji
Copy link
Member

@almostSouji almostSouji commented Apr 6, 2021

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

Implement Guild#fetchWelcomeScreen, Guild#editWelcomeScreen, add welcome screen data to invites retrieved via invite fetching (Client#fetchInvite) as per:

Notes:

  • (reverted) QoL change for BaseGuildEmojiManager to accept the <:name:id> and name:id syntax in resolve* methods
  • ⚠️ Due to the nature of the emoji data we receive, this requires for Emoji#animated to potentially return null, making this a semver major PR. (upstream issue see below, not too much hope on that happening, though)
  • ⚠ Changing the return value of Invite#guild (major)
  • implement "enabled" via getter from guild features to not rely on upstream
  • in-function reply is a hacky workaround for circular dependencies
  • introduce intermediary, abstract guild to not dupe code for Guild and InviteGuild

Upstream:

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • This PR changes the library's interface (methods or parameters added)
  • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/WelcomeChannel.js Outdated Show resolved Hide resolved
src/structures/WelcomeChannel.js Outdated Show resolved Hide resolved
src/structures/WelcomeChannel.js Outdated Show resolved Hide resolved
src/structures/WelcomeScreen.js Outdated Show resolved Hide resolved
src/structures/WelcomeScreen.js Outdated Show resolved Hide resolved
src/structures/WelcomeScreen.js Outdated Show resolved Hide resolved
src/structures/WelcomeScreen.js Outdated Show resolved Hide resolved
typings/index.d.ts Show resolved Hide resolved
@almostSouji almostSouji marked this pull request as draft April 10, 2021 18:41
@almostSouji almostSouji marked this pull request as ready for review April 10, 2021 19:40
src/managers/BaseGuildEmojiManager.js Outdated Show resolved Hide resolved
src/managers/BaseGuildEmojiManager.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
src/structures/Emoji.js Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
@SpaceEEC
Copy link
Member

Is it intentional to not populate / overwrite / update Guild#welcomeScreen when fetching or editing it? If yes, why?

@almostSouji
Copy link
Member Author

Not too sure how to handle any of this for what it's worth

  • we don't get a GUILD_UPDATE event for when this thing updates
  • guilds will only ever have a welcomeScreen if they arrive via an invite (which also creates pseudo/partial/very empty guilds based on the little information we get from it, but doesn't cache them)
  • what even happens if we have a guild cached and then fetch an invite from it?
  • if we populate and update #wecomeScreen on fetch/edit that data immediately becomes out of date, since we don't receive any WS events to continuously update the screen based on

An entirely different approach could be to emit an InviteGuild which is directly attached to invites and reflects the guilds state at the point the invite was created/arrived, so we don't have outdated data in caches.

This problem also arises with interactions and the new #permissions field which reflects permissions after overwrites are applied in the channel the interaction was used in at the time the interaction was used.

src/structures/Guild.js Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/WelcomeScreen.js Outdated Show resolved Hide resolved
@almostSouji
Copy link
Member Author

almostSouji commented Apr 30, 2021

how should i handle welcome screen data from invite guilds?
a) cache and update when the screen is fetched
b) cache and don't update
c) build invites with an InviteGuild, which has that property and extends Guild, cache without it as a regular guild (if from a server the bot is on)

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.

Only thing I have to say is #5490 (comment)

@vladfrangu
Copy link
Member

how should i handle welcome screen data from invite guilds?
a) cache and update when the screen is fetched
b) cache and don't update
c) build invites with an InviteGuild, which has that property and extends Guild, cache without it as a regular guild (if from a server the bot is on)

Option c sounds ideal, but I don't know what others think. @iCrawl @kyranet?

@iCrawl
Copy link
Member

iCrawl commented May 6, 2021

Option C sounds reasonable (maybe a bit overkill, but what isn't in the current state of the library) and in line with the pattern we do it for other resources.

@kyranet
Copy link
Member

kyranet commented May 7, 2021

There are merge conflicts in this PR, please rebase @almostSouji.

Also, since this seems blocked/pending review resolution, should this be marked as draft?

@almostSouji almostSouji marked this pull request as draft May 7, 2021 16:32
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.

Maybe add a mention of InviteGuild in BaseGuild too (and mark it as @abstract) while are at it? 😅

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

I think BaseGuild also needs an abstract in the typings.

src/structures/BaseGuild.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
@almostSouji
Copy link
Member Author

reverted accepting broader emoji data after internal discussion following proposal #5839

@iCrawl iCrawl merged commit 44e2ee7 into discordjs:master Jun 19, 2021
@almostSouji almostSouji deleted the welcome-screen branch June 19, 2021 15:53
@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
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