Closed chrissantamaria closed 10 months ago
Latest commit: c8c4e16719222e284eea9e662295aa9e071c97a7
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.
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
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
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.
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:
const results = await group()
.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!
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.
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.
Why hasn't it been merged?
@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!
@chrissantamaria Is it possible you can add changesets
to the pr ? Just run npx changesets --empty
Thanks!
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 asValue
. 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!