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

Added MessageEmbed Typings #3492

Open
wants to merge 3 commits into
base: master
from

Conversation

@Tenpi
Copy link
Contributor

commented Sep 28, 2019

Please describe the changes this PR makes and why it should be merged:
Discord.js stable had typings for MessageEmbedThumbnail, MessageEmbedAuthor, etc. However, they were removed entirely in the master branch. I have been defining these types in my own project, but I believe that they should already be defined by the library. I'm sure there was a reason for the removal, but I want to propose to add these types back since it forces Typescript users to either define it themselves, or to use the any type. (also this is my first PR, sorry if I did anything wrong).

Here are my additions:
-Added types for MessageEmbed Author, Thumbnail, Image, Video, Provider, and Footer
-The type EmbedField was already defined, but for some reason { name: string; value: string; inline?: boolean; } was used instead (changed it to EmbedField)
-Updated the docs in MessageEmbed.js

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.
Tenpi added 2 commits Sep 28, 2019
@SpaceEEC SpaceEEC added the t: typings label Oct 2, 2019
@Tenpi

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

I re-looked this over and noticed some inconsistencies in the typings. For example, author in MessageEmbedOptions was defined as such:

{ 
    name?: string; 
    url?: string; 
    icon_url?: string; 
    iconURL?: string; 
}

And in MessageEmbed it is defined as:

{
    name?: string;
    url?: string;
    iconURL?: string;
    proxyIconURL?: string;
}

To me this looks like a mistake, since no other property uses snake_case and since icon_url and iconURL are essentially identical names. However, I will revert the changes if I am incorrect.

Copy link
Member

left a comment

I think what you noticed there seems correct / intended.

MessageEmbeds themselves only have camelCase properties.

However its constructor should accept both:

  • snake_case variants are for data received from Discord (e.g., from WS / HTTP)
  • camelCase variants are for MessageEmbeds (e.g. when cloning)

Making a type intersection in MessageEmbedOptions "adding" those snake_case variants should be sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.