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

types: make application command option union easier to use #250

Merged
merged 5 commits into from
Dec 22, 2021

Conversation

suneettipirneni
Copy link
Member

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

The previous way that autocomplete was enforced create types that were hard/near impossible to use. This PR sacrifices the type strength for usability making it waaay easier to adopt. So essentially choices and autocomplete are no longer mutually exclusive.

@vladfrangu
Copy link
Member

I don't understand how this fixes anything. If anything, you should've added autocomplete?: false to the options that can also have choices, instead of merging them again in one type

@suneettipirneni
Copy link
Member Author

I don't understand how this fixes anything. If anything, you should've added autocomplete?: false to the options that can also have choices, instead of merging them again in one type

That won't work because of the way discriminated unions work.

In our case we use type as the tag for each union. If we split the autocomplete options up and the choices options up, we'll end up with two interfaces that both have type set to string number or integer.

Because there's two interfaces with the same type values. It breaks the union and it'll no longer work properly.

@vladfrangu
Copy link
Member

vladfrangu commented Dec 1, 2021

No it won't? Playground Link

image

Are you sure this is breaking anything? Could you build a playground url where it breaks?

Even then, I am not merging this PR in its current state as you removed min and max_values from number options.

@suneettipirneni
Copy link
Member Author

No it won't? Playground Link

image

Are you sure this is breaking anything? Could you build a playground url where it breaks?

Even then, I am not merging this PR in its current state as you removed min and max_values from number options.

Here's a playground that shows what I'm talking about:

playground link

@vladfrangu
Copy link
Member

Easy fix. Playground Link

@suneettipirneni
Copy link
Member Author

Easy fix. Playground Link

🤦‍♂️ I'm dumb

@vladfrangu
Copy link
Member

Are you sure that fixes it? because it also looks wrong

@kyranet
Copy link
Member

kyranet commented Dec 1, 2021

When in doubt, add tests, lots of type tests. That should tell us whether or not they're fixed.

Although... yeah. We first need a unit test suite in this repository.

@vladfrangu
Copy link
Member

How do you unit test... types? Also, we do have tests, with tsd, in the tests folder

@kyranet
Copy link
Member

kyranet commented Dec 1, 2021

Oh, I didn't see that directory.

How do you unit test... types?

Just like how we have been unit testing types in the main library for months? Even this project's tests test types:

if (interaction.type === InteractionType.MessageComponent) {
expectType<APIMessageComponentInteraction>(interaction);

@vladfrangu vladfrangu merged commit 8bbb819 into discordjs:main Dec 22, 2021
@suneettipirneni suneettipirneni deleted the types/autocomplete-fix branch December 22, 2021 18:20
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.

None yet

3 participants