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(TypeGuards): add TypeGuards utility for commonly used data types #6410

Closed

Conversation

suneettipirneni
Copy link
Member

@suneettipirneni suneettipirneni commented Aug 12, 2021

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

Allows for easier type narrowing, for example:

function consumeOption(option: ApplicationCommandOptionData) {
   if (TypeGuards.optionDataSupportsChoices(option)) {
     // Has an `options` field.
     option.choices.forEach( ... )
   } else {
     // Doesn't have an `options` field.
   }
}

Closes #6409

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)

@iCrawl iCrawl added this to the Version 13.2 milestone Aug 13, 2021
src/util/TypeGuards.js Outdated Show resolved Hide resolved
src/util/TypeGuards.js Outdated Show resolved Hide resolved
src/util/TypeGuards.js Outdated Show resolved Hide resolved
src/util/TypeGuards.js Outdated Show resolved Hide resolved
src/util/TypeGuards.js Outdated Show resolved Hide resolved
src/util/TypeGuards.js Outdated Show resolved Hide resolved
src/util/TypeGuards.js Outdated Show resolved Hide resolved
src/util/TypeGuards.js Outdated Show resolved Hide resolved
suneettipirneni and others added 7 commits August 13, 2021 03:13
Co-authored-by: Shubham Parihar <shubhamparihar391@gmail.com>
Co-authored-by: Shubham Parihar <shubhamparihar391@gmail.com>
Co-authored-by: Shubham Parihar <shubhamparihar391@gmail.com>
Co-authored-by: Shubham Parihar <shubhamparihar391@gmail.com>
Co-authored-by: Shubham Parihar <shubhamparihar391@gmail.com>
Co-authored-by: Shubham Parihar <shubhamparihar391@gmail.com>
Co-authored-by: Shubham Parihar <shubhamparihar391@gmail.com>
src/util/TypeGuards.js Outdated Show resolved Hide resolved
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.

I don't know if this is needed, since TS can infer from the properties if we have done unions right, but LGTM

src/util/TypeGuards.js Outdated Show resolved Hide resolved
@suneettipirneni
Copy link
Member Author

suneettipirneni commented Aug 13, 2021

I don't know if this is needed, since TS can infer from the properties if we have done unions right, but LGTM

You're correct, but I find doing having to do command.type === 'CHAT_INPUT' && command.type === Constants.ApplicationCommandTypes.CHAT_INPUT, is too verbose, and somewhat confusing. The TypeGuards in this context are more like aliases for these checks.

An alternative would be to split up the interfaces even more (add another interface for each variant, one for strings one for enums). But I dunno how I feel about that either.

@suneettipirneni
Copy link
Member Author

On reevaluation it seems that command.type === 'TYPE' does work as intended without checking enum values, so I'll remove the command checks but keep the option/choice checking as those are still useful.

typings/index.d.ts Outdated Show resolved Hide resolved
typings/index.d.ts Show resolved Hide resolved
src/util/TypeGuards.js Outdated Show resolved Hide resolved
src/util/TypeGuards.js Outdated Show resolved Hide resolved
src/util/TypeGuards.js Outdated Show resolved Hide resolved
src/util/TypeGuards.js Outdated Show resolved Hide resolved
src/util/TypeGuards.js Outdated Show resolved Hide resolved
typings/index.d.ts Outdated Show resolved Hide resolved
@iCrawl
Copy link
Member

iCrawl commented Aug 25, 2021

I think something like this might be better suited for @discordjs/typeguards or something, unsure about having this in the main lib.

@suneettipirneni
Copy link
Member Author

That's fine by me. Should I close this now and wait for that repo to be published?

@iCrawl
Copy link
Member

iCrawl commented Aug 25, 2021

Our modules are usually in the https://github.com/discordjs/discord.js-modules repo.

You could theoretically look at the REST module and branch out your own if you wanted to in a PR. All we require is it having tests and the structure you can see how REST is built.

@suneettipirneni
Copy link
Member Author

Sweet! I'll take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add utility type guards for easier type narrowing
9 participants