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(Interactions): add autocomplete api types #205

Merged
merged 18 commits into from
Oct 29, 2021

Conversation

suneettipirneni
Copy link
Member

@suneettipirneni suneettipirneni commented Sep 23, 2021

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

Adds very experimental support for autocomplete.

Reference Discord API Docs PRs or commits:

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.

Initial review - don't forget v8 :D

rest/v9/webhook.ts Outdated Show resolved Hide resolved
payloads/v9/interactions.ts Outdated Show resolved Hide resolved
payloads/v9/_interactions/responses.ts Show resolved Hide resolved
@suneettipirneni
Copy link
Member Author

suneettipirneni commented Sep 23, 2021

So unfortunately typescript won't be able to enforce that the command has either a choices array OR a autocomplete boolean, without breaking the existing union. So it's just going to be appended onto the APIApplicationCommandArgumentOptions and will need to be enforced during runtime.

@ckohen
Copy link
Member

ckohen commented Sep 23, 2021

When sending only typescript can enforce either choices or autocomplete via a discriminated union. On the receiving side there's not much of a benefit to having a discriminated union, but it could still work, just means people have to build type guards

@suneettipirneni
Copy link
Member Author

When sending only typescript can enforce either choices or autocomplete via a discriminated union. On the receiving side there's not much of a benefit to having a discriminated union, but it could still work, just means people have to build type guards

Even on the sending side with a discriminated union it doesn't work properly, it's because TS unions aren't 'exclusive or' unions. That means if multiple types with the same tag (type) in this case are used, the discriminated union will no longer function properly because there's no longer a 1-1 mapping of tags to types.

@ckohen
Copy link
Member

ckohen commented Sep 23, 2021

You need a discriminated union explicitly preventing the other property. But I realized there's no identifier for which we would be using without adding a nonexistent prop. How does discord handle it if you send both anyways? Maybe a type generic could work?

@suneettipirneni
Copy link
Member Author

You need a discriminated union explicitly preventing the other property. But I realized there's no identifier for which we would be using without adding a nonexistent prop. How does discord handle it if you send both anyways? Maybe a type generic could work?

Maybe I'm missing something, but I don't know how a generic type would help here, since generics don't change which fields are shown and which ones are not shown. Generics only affect already existing fields. As for how discord handles it I'm not entirely sure since I haven't tested it, but according the docs PR, choices can't be present when autocomplete is true.

@ckohen
Copy link
Member

ckohen commented Sep 23, 2021

This is what I mean by using generics

@suneettipirneni
Copy link
Member Author

This is what I mean by using generics

I see how this can work, however, how would you integrate this into the APIApplicationCommandOption type union?

@suneettipirneni
Copy link
Member Author

Alright so, somehow I was able to get typescript to typecheck the autocomplete variants. I basically made the autocomplete property of type never on all types except on choice types. When choices are provided, autocomplete is defined as false or undefined. If autocomplete is present choices is defined as never. This results in invalid variants not being allowed by typescript.

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.

More issues, please also port the fixes to v9 :D

payloads/v8/_interactions/autocomplete.ts Outdated Show resolved Hide resolved
payloads/v8/_interactions/autocomplete.ts Outdated Show resolved Hide resolved
payloads/v8/_interactions/responses.ts Outdated Show resolved Hide resolved
rest/v8/interactions.ts Outdated Show resolved Hide resolved
@suneettipirneni suneettipirneni marked this pull request as ready for review October 21, 2021 15:01
@vladfrangu vladfrangu changed the title feat: add autocomplete api types feat(Interactions): add autocomplete api types Oct 29, 2021
@vladfrangu vladfrangu merged commit 3b9320d into discordjs:main Oct 29, 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

4 participants