feat: export Question and Answers #1559
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1559 +/- ##
=======================================
Coverage ? 98.03%
=======================================
Files ? 39
Lines ? 2389
Branches ? 644
=======================================
Hits ? 2342
Misses ? 40
Partials ? 7 ☔ View full report in Codecov by Sentry. |
mshima
force-pushed
the
export-types
branch
from
8a86f7a
to
3f3d96b
Compare
mshima
force-pushed
the
export-types
branch
from
3f3d96b
to
b0606d7
Compare
| @@ -47,7 +47,7 @@ export interface QuestionMap { | |||
| type KeyValueOrAsyncGetterFunction<T, k extends string, A extends Answers> = | |||
| T extends Record<string, any> ? T[k] | AsyncGetterFunction<T[k], A> : never; | |||
|
|
|||
| export type AnyQuestion<A extends Answers, Type extends string = string> = { | |||
| export type AnyQuestion<A extends Answers = Answers, Type extends string = string> = { | |||
There was a problem hiding this comment.
Do we want to rename it to match what @type/inquirer used?
There was a problem hiding this comment.
AnyQuestion is been exported as Question.
Answers matches too.
Question has a more advanced type called DistinctQuestion which is the current BuiltInQuestion with name property.
I prefer BuiltInQuestion over DistinctQuestion.
There was a problem hiding this comment.
DistinctQuestion was used in @types/inquirer. I honestly do not care so long as we get that type(BuiltInQuestion or DistinctQuestion, wherever the decision lands), but if its all the same, i'd use the same one that was used before for the sake of "easier" transition.
packages/inquirer/inquirer.test.mts
Outdated
| describe('exported types', () => { | ||
| type Answers = import('./src/index.mjs').Answers; | ||
| type Question = import('./src/index.mjs').Question; | ||
|
|
||
| it('Question type is not any', () => { | ||
| expectTypeOf({}).not.toMatchTypeOf<Question>(); | ||
| }); | ||
|
|
||
| it('exported Question type requires type, name and message', () => { | ||
| expectTypeOf({ | ||
| type: 'stub', | ||
| name: 'q1', | ||
| message: 'message', | ||
| }).toMatchTypeOf<Question>(); | ||
| expectTypeOf({ name: 'q1', message: 'message' }).not.toMatchTypeOf<Question>(); | ||
| expectTypeOf({ type: 'stub', message: 'message' }).not.toMatchTypeOf<Question>(); | ||
| expectTypeOf({ type: 'stub', name: 'q1' }).not.toMatchTypeOf<Question>(); | ||
| }); | ||
|
|
||
| it('exported Answers type is not any', () => { | ||
| expectTypeOf(false).not.toMatchTypeOf<Answers>(); | ||
| }); | ||
|
|
||
| it('exported Answers type matches any object', () => { | ||
| expectTypeOf({}).toMatchTypeOf<Answers>(); | ||
| expectTypeOf({ foo: 'bar' }).toMatchTypeOf<Answers>(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
In term of test, I was more thinking along the line of ensuring proper behavior when used; e.g
it('Question[] infers the right return type', async () => {
const questions: Question[] = [
{ ... },
];
const answers = await inquirer.prompt(questions);
expectTypeOf(answers).toMatchTypeOf<{ foo: any; bar: any; }>();
});There was a problem hiding this comment.
I am not sure it's possible.
prompt() function types are too complex to be reused.
Types are internal to the prompt function.
I think we should expose a generic type.
There was a problem hiding this comment.
I don't think I follow - I'm just talking of testing the behavior, not changing the type interface (unless it'd become necessary.)
There was a problem hiding this comment.
Returned type of Question will alway be Record<string, any>.
There was a problem hiding this comment.
Ah yes, this is what I thought would happen. That's what I was telling @DelliriumX over #1527 (comment) - but they seemed pretty sure of themselves 🤷🏻 Figured I was missing something.
So yeah then exporting the types (as-is) does break type inference.
There was a problem hiding this comment.
- I am pretty sure answers type inference does not exist in v9 types.
- in yeoman-generator there is no type inference and I don’t plan to add it.
Exported types is just for easy custom apis:
- if any type inference is needed it should use the function type.
- It’s not even possible for inquirer to create the function type, maybe it’s not possible to extract it and keep type inference.
Plain Question/Answers and maybe BuiltInQuestion should be enough to replace @types/inquirer.
There was a problem hiding this comment.
Ah yes, this is what I thought would happen. That's what I was telling @DelliriumX over #1527 (comment) - but they seemed pretty sure of themselves 🤷🏻 Figured I was missing something.
So yeah then exporting the types (as-is) does break type inference.
Do not use the Question use what was equivalent to the BuiltInQuestion used inside the prompt. If I can make a type that behaves properly with 1 line of code, inferring from prompt then I see no reason why this cannot be exported from the package.
I think i know what you are getting at. Let me see if i can somehow make it stick
Edit: Okay so here's the conclusion i arrived to after inspecting it a bit more thoroughly.
Types used in the prompt are significantly more complex then the ones I used in the example demonstrated on that comment . There is no way for us to gain the benefits of both inference AND external typing at the same time, but this does not mean that "exporting types (as-is) breaks inference". That said, exporting types wont break inference (using them will break it for that particular function call), though I think what @mshima noted here is a very good point.
With that in mind, people will be able to choose between:
- getting inference and inline questions objects
- typing questions and typing answers through generics
At least I think this is what the outcome may end up being. Seeing as how with Question/BuiltInQuestion and Answer we can replicate the v9 behaviour. I think that these should be enough to replace the @types/inquirer as it stands right now (well, at least for my use-case it will) because most uses were through DistinctQuestion and that, to my knowledge, had no answer type inference whatsoever - so it should be fine when coupled with Answer replacement for generic
Related to #1527.