-
Notifications
You must be signed in to change notification settings - Fork 61
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
chore: Convert simple prompts to TypeScript #1273
Conversation
// Accounts for deprecated and new config | ||
const defaultAccount = getConfigDefaultAccount(config); | ||
const defaultAccount = getConfigDefaultAccount(); |
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.
getConfigDefaultAccount
doesn't take any arguments--the power of TypeScript!
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.
Looking good! Just a few minor comments
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.
One tiny thing I noticed but otherwise LGTM
Description and Context
In this PR, I convert simple prompts to TypeScript. These include:
accountNamePrompt.ts
cleanUploadPrompt.ts
createFunctionPrompt.ts
createModulePrompt.ts
createTemplatePrompt.ts
projectsLogsPrompt.ts
secretPrompt.ts
setAsDefaultAccountPrompt.ts
IMPORTANT: I was having trouble importing inquirer when converting prompts to TS. We're currently using inquirer v.8.2.0, which is a CommonJS module. We need to bump to the current version (v.12.1.0) to import it using ES module syntax.
Currently, we're wrapping inquirer.createPromptModule() in our helper function, promptUser. My gut feeling is that we should convert promptUser to inquirer.prompt in one PR, once all the files that consume promptUser are converted to TS. Is that a reasonable plan?
I don't see how we can support two inquirer versions (one CommonJS andv one ES module syntax), but let me know if that's possible/preferable.
TODO
inquirer
Who to Notify
@camden11 @brandenrodgers @joe-yeager