clack icon indicating copy to clipboard operation
clack copied to clipboard

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

Open kevduc opened this issue 2 years ago • 4 comments

This PR addresses this issue https://github.com/natemoo-re/clack/issues/144.

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

kevduc avatar Aug 17 '23 02:08 kevduc

⚠️ No Changeset found

Latest commit: f663d0f7e2a37ba7a088ac08f92c9e13a72dffe7

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

changeset-bot[bot] avatar Aug 17 '23 02:08 changeset-bot[bot]

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

Example

kevduc avatar Aug 17 '23 19:08 kevduc

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 avatar Aug 23 '23 13:08 cpreston321

@cpreston321 Thanks for the suggestions!
The fix for MultiSelectOptions is pretty much the same as for select (https://github.com/natemoo-re/clack/pull/145/commits/20cde30e5e48fbe2b15a8d5dba2ae8f9845a4489). 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 https://github.com/natemoo-re/clack/pull/145/commits/f663d0f7e2a37ba7a088ac08f92c9e13a72dffe7: 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 avatar Aug 28 '23 01:08 kevduc