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

fix(@clack/core,@clack/prompts): add non empty opts type interface #145

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kevduc
Copy link

@kevduc kevduc commented Aug 17, 2023

This PR addresses this issue #144.

The current implementation is erroring out at runtime, the implementation in this PR shows a type error in the IDE/tsc instead.

@changeset-bot
Copy link

changeset-bot bot commented Aug 17, 2023

⚠️ No Changeset found

Latest commit: f663d0f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@kevduc kevduc marked this pull request as draft August 17, 2023 02:12
@kevduc kevduc changed the title Fix SelectPrompt options type Fix SelectPrompt and select options type Aug 17, 2023
@kevduc kevduc marked this pull request as ready for review August 17, 2023 02:46
@kevduc
Copy link
Author

kevduc commented Aug 17, 2023

Example of what the type error could look like in VSCode:

Example

@cpreston321
Copy link
Collaborator

cpreston321 commented Aug 23, 2023

Hey @kevduc ! Thanks for this PR! 🙏🏼

I recently merged a PR to simplify types on @clack/prompts if you want to resolve issues -- I can then be able to merge this!

I could also see this being used also on https://github.com/natemoo-re/clack/blob/7c7fde8dcdfa1edc3feb131331290affbaa6d934/packages/prompts/src/index.ts#L307 & https://github.com/natemoo-re/clack/blob/7c7fde8dcdfa1edc3feb131331290affbaa6d934/packages/prompts/src/index.ts#L423

@cpreston321 cpreston321 self-requested a review August 23, 2023 13:31
@cpreston321 cpreston321 changed the title Fix SelectPrompt and select options type fix(@clack/core,@clack/prompts): add non empty opts type interface Aug 24, 2023
@kevduc kevduc force-pushed the fix-empty-options-array-for-selectprompt branch from 36cd1ad to 93fc16a Compare August 24, 2023 13:54
@kevduc kevduc marked this pull request as draft August 24, 2023 13:55
@kevduc
Copy link
Author

kevduc commented Aug 28, 2023

@cpreston321 Thanks for the suggestions!
The fix for MultiSelectOptions is pretty much the same as for select (20cde30).
I also had a look at GroupMultiSelectPrompt:

  • it looks like it doesn't make sense to have an empty list of options for a given group (and it currenlty is buggy, see https://stackblitz.com/edit/node-51onk7?file=index.ts, the test3 group looks selected, it can't be toggled, and when pressing enter it says nothing is selected), so the typing is fixed in f663d0f:
    2023-08-28 02_22_19-Window
  • it also looks like there's another bug when options itself is empty (see https://stackblitz.com/edit/node-dhmjec?file=index.ts, press space to make it crash):
    TypeError: Cannot read properties of undefined (reading 'group')
        at CD.toggleValue (file:///home/projects/node-dhmjec/node_modules/@clack/core/dist/index.mjs:40:1272)
        at Object.eval [as cb] (file:///home/projects/node-dhmjec/node_modules/@clack/core/dist/index.mjs:40:1049)
        at CD.emit (file:///home/projects/node-dhmjec/node_modules/@clack/core/dist/index.mjs:35:1851)
        at CD.onKeypress (file:///home/projects/node-dhmjec/node_modules/@clack/core/dist/index.mjs:35:2130)
    
    I tried an approach where I use a NonEmptyObject type on options in GroupMultiSelectOptions, i.e. something like this:
    // packages\core\src\utility-types.ts
    export type NonEmptyObject<T> = keyof T extends never ? never : T;
    
    // packages\core\src\prompts\group-multiselect.ts
    interface GroupMultiSelectOptions<
        T extends { value: any },
        U extends Record<string, NonEmptyArray<T>> = Record<string, NonEmptyArray<T>>
    > extends PromptOptions<GroupMultiSelectPrompt<T, U>> {
        options: NonEmptyObject<U>;
        initialValues?: T['value'][];
        required?: boolean;
        cursorAt?: T['value'];
    }
    2023-08-28 01_18_12-Window
    but that pretty much breaks the inference of Value for groupMultiselect (it ends up always being unknown) and I didn't manage to make it work (might need to refactor to have the type of value as a generic instead of the type of option { value: any }), so I've not added it to this MR, and if there's no straightforward fix, that's something that can be looked at in a different MR. An idea would be to at least do a runtime check and throw a more meaningful error.

@kevduc kevduc marked this pull request as ready for review August 28, 2023 01:07
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.

2 participants