Fix complex value return type inference

[chrissantamaria]

This PR addresses in an issue discussed in #44, where select fields with non-primitive values (originally introduced in #86) are not properly inferring the type of the returned user selection.

I did so by removing the Options generic from all fields as, from what I can tell, it's effectively representing the same constraint as Value. Typechecking for the build still passes, but there's a chance I broke type inference in some other use case - please double check me here!

[changeset-bot]

⚠️ No Changeset found

Latest commit: c8c4e16

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

[ulken]

I've looked at this and played around a bit. It's definitely an improvement. The previous abstraction was unnecessary.

However, it doesn't seem to work with group (check the type of the "type" select in the basic example). Not sure why. Care to work your magic there as well? :p

[ulken]

To clarify: this PR doesn't break the type inference for group, it was already broken.

So this could be merged as an improvement even if group remains broken.

[chrissantamaria]

Did some testing with this - it looks like in basic cases, groups should just work™! Take this example from the docs:

Complex values work as well, like in the example from #44:

select + multiselect can even give back nice type unions of literals:

However, the fun comes in when you try to use results from a previous step:

This unfortunately makes sense - the resulting type of the multiselect could in theory be dependent on any of the properties from results, including itself, resulting in a circular type => unknown :(

There's also the unfortunate reality that each of the values in results could be undefined, since TypeScript can't guarantee that they've already been set before the current step:

In a world of no backwards compat, it might be simpler to structure groups with a builder-style pattern. Something like:

constresults=awaitgroup().addStep('name',()=>text({message: 'What is your name?'})).addStep('age',()=>text({message: 'What is your age?'})).addStep('color',({ results })=>multiselect({message: `What is your favorite color ${results.name}?`,options: [{value: 'red',label: 'Red'},{value: 'green',label: 'Green'},{value: 'blue',label: 'Blue'},],})).prompt();

This way, the type of values can be widened on each step rather than always including all steps with optional values types.

How we move forward here is really up to you all. I understand if we don't want to make any breaking API changes, but I don't see how to feasibly wrangle the types here. Then again, there's definitely a good chance I'm misinterpreting the root issue here, so I'm open to other thoughts!

Though I think this issue is important (and should likely be tracked via a separate issue), imo this PR is unrelated and should be handled separately. Unless I'm missing anything, this one should be ready to go!

[ulken]

the resulting type of the multiselect could in theory be dependent on any of the properties from results, including itself, resulting in a circular type => unknown

Does unknown solely stem from the fact that the current step can be referenced? If so, since we're now omitting that from the partial results, does that solve the problem?

If not, I'll go ahead and merge this.

[chrissantamaria]

hmm, looks like my theory was wrong - just rebased with the latest main and still getting the same issue :(

TS should have all the info it needs to infer types, but I wonder if it bails as soon as any individual prompts consume results - I suppose there's a chance of circular dependencies and TS might not be feasibly able to detect those instances.

Regardless, I agree this should be good to merge (feel free to add a changeset if you'd like) - let's track in a separate issue.

[orochaa]

Why hasn't it been merged?

[cpreston321]

@Mist3rBru I wanted to merge this! I wanted to make sure there hasn't been any conflicts with the changes from the changes merged is latest release. I will look into this today!

[cpreston321]

@chrissantamaria Is it possible you can add changesets to the pr ? Just run npx changesets --empty

Thanks!

[cpreston321]

LGTM