-
Notifications
You must be signed in to change notification settings - Fork 77
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 prompt handler type #617
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -196,7 +195,7 @@ declare module '@gasket/engine' { | |||
prompt( | |||
context: CreateContext, | |||
utils: { | |||
prompt: Inquirer, | |||
prompt: (prompts: Array<Record<string, any>>) => Promise<Record<string,any>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem like a very helpful type. Can we find a better way to import the type instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or add @types/inquirer
as a dependency so it is exported? Hmm, looks like maybe there was a version bump and the types don't align now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like @types/inquirer
has a PromptFunction
type that maybe is what this is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We set up the prompt function conditionally here. We are using inquirer@6 in the CLI currently, which does not export types. There is a @types/inquirer
, but then it becomes a necessary dependency for users to add, unless we make it a direct dependency of the CLI, which doesn't feel right either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will investigate further when upgrading
#618
Summary
More suitable type declaration which is not dependent on deep dependencies.
Fixes
error TS2305: Module '"inquirer"' has no exported member 'Inquirer'.
Changelog
@gasket/cli
Test Plan
Tested change in local app.