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: Make message components agnostic components #7040

Closed

Conversation

suneettipirneni
Copy link
Member

@suneettipirneni suneettipirneni commented Nov 26, 2021

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

This deprecates all other Message* types, so this shouldn't be breaking.

It's breaking

This makes all components regardless of being a message component, or a modal component all inherit from the same base, and have no difference in variance between message and modal implementations. This is key refactor needed for the modal PR (#7023), and prevents future class proliferation of other component type variants.

Status and versioning classification:

  • 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
  • This PR changes the library's interface (methods or parameters added)

src/ComponentFactory.js Outdated Show resolved Hide resolved
src/structures/ActionRow.js Show resolved Hide resolved
src/structures/ActionRow.js Show resolved Hide resolved
src/structures/BaseComponent.js Show resolved Hide resolved
src/structures/BaseMessageComponent.js Outdated Show resolved Hide resolved
src/structures/MessageSelectMenu.js Outdated Show resolved Hide resolved
src/util/Constants.js Show resolved Hide resolved
src/util/Constants.js Outdated Show resolved Hide resolved
typings/index.test-d.ts Outdated Show resolved Hide resolved
typings/index.test-d.ts Outdated Show resolved Hide resolved
src/structures/SelectMenuComponent.js Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
src/structures/ActionRow.js Show resolved Hide resolved
src/structures/ActionRow.js Show resolved Hide resolved
src/structures/ButtonComponent.js Show resolved Hide resolved
src/util/Constants.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/structures/ActionRow.js Show resolved Hide resolved
@suneettipirneni
Copy link
Member Author

Just realized that this is going to need to be semver: major, because it changes the type of Message#components.

@ImRodry
Copy link
Contributor

ImRodry commented Nov 27, 2021

I don’t think there’s a use case where that would break though is there?

@suneettipirneni
Copy link
Member Author

I don’t think there’s a use case where that would break though is there?

The only place I can think of is using instanceof checks. Although doing these types of checks is pointless anyways. But I'll defer this to @discordjs/the-big-4

@ImRodry
Copy link
Contributor

ImRodry commented Nov 27, 2021

Yeah there’s always that but I don’t think anyone would be doing this here. You can also make the old classes extend the new ones and I think the instanceof checks wouldn’t break here, right? This would work since I believe all properties and methods are the same right

@suneettipirneni
Copy link
Member Author

If I made the new classes extend the old ones it would defeat the purpose of this PR.

@ImRodry
Copy link
Contributor

ImRodry commented Nov 27, 2021

My suggestion is to make the old ones extend the new ones. The properties are the same, but documented differently (at least how I see it) so it wouldn’t defeat the purpose would it?

@suneettipirneni
Copy link
Member Author

My suggestion is to make the old ones extend the new ones. The properties are the same, but documented differently (at least how I see it) so it wouldn’t defeat the purpose would it?

Perhaps, but then you might run into the issue where instanceof BaseMessageComponent won't work. 🤔

@ImRodry
Copy link
Contributor

ImRodry commented Nov 27, 2021

I don't think anyone in their right mind would do such a thing but yeah you're right :/
Can we ignore that small nit?

@iCrawl iCrawl requested a review from SpaceEEC November 29, 2021 10:17
@iCrawl iCrawl added this to the Version 14 milestone Dec 1, 2021
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.

Small nit otherwise LGTM to me

src/structures/ActionRow.js Outdated Show resolved Hide resolved
@kyranet
Copy link
Member

kyranet commented Dec 7, 2021

Please rebase, there are merge conflicts.

@iCrawl
Copy link
Member

iCrawl commented Dec 24, 2021

This needs a rebease.

@suneettipirneni
Copy link
Member Author

suneettipirneni commented Dec 26, 2021

So I'm probably going to close this PR and open another one later that uses the message components from builders instead.

@suneettipirneni
Copy link
Member Author

Superceded by #7195

@suneettipirneni suneettipirneni deleted the refactor/components branch February 7, 2022 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants