-
Notifications
You must be signed in to change notification settings - Fork 617
feat(cli): Add interactive prompts for commands #5170
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
Conversation
🤖 My Senior Dev — Analysis Complete👤 For @agusayerza📁 Expert in View your contributor analytics → 📊 13 files reviewed • 10 need attention
🚀 Open Interactive Review →The full interface unlocks features not available in GitHub:
💬 Chat here: 📖 View all 12 personas & slash commandsYou can interact with me by mentioning In PR comments or on any line of code:
Slash commands:
AI Personas (mention to get their perspective):
For the best experience, view this PR on myseniordev.com — includes AI chat, file annotations, and interactive reviews. |
| console.error(chalk.red(parsing.error)); | ||
| } | ||
| } | ||
| } |
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.
would it make sense to pass the integrations as argument and move the the parsing/loading outside of the interactive logic? Same for fetching the connections below?
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.
Done. I moved the parsing logic out and just pass the list to the prompt function now
| type: 'rawlist', | ||
| name: 'env', | ||
| message: 'Which environment do you want to use?', | ||
| choices: ['dev', 'prod', new inquirer.Separator(), OTHER_CHOICE] |
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.
why not fetching the environments from the API?
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.
Checked the API and there is no endpoint to list environments yet, maybe I missed it? I talked with Bastien of this being a fast-follow-up
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.
Just checked the code, endpoint exists but not documented or exposed on the SDK. My bad. I still think this is an improvement to be done as a fast-follow-up
packages/cli/lib/index.ts
Outdated
| cmd.option('--auto-confirm', 'Auto confirm yes to all prompts.', false); | ||
| cmd.option('--debug', 'Run cli in debug mode, outputting verbose logs.', false); | ||
| // Default to true so that interactive mode is enabled by default | ||
| cmd.option('--no-interactive', 'Disable interactive prompts for missing arguments.', true); |
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.
not sure I understand why --no-interactive is true by default if we want the interactive mode enabled by default
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.
--no prefix on commander sets the flag to false when you pass it instead of true. Added a better description to the comment to explain that weird behaviour
| .action(async function (this: Command) { | ||
| const { debug, ai, copy } = this.opts<GlobalOptions & { ai: string[]; copy: boolean }>(); | ||
| const { debug, ai, copy, interactive } = this.opts<GlobalOptions & { ai: string[]; copy: boolean }>(); | ||
| let [projectPath] = this.args; |
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.
where is projecPath being used?
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.
Good catch. It's being used now to resolve the absolute path, missed it
packages/cli/lib/index.ts
Outdated
| if (!functionType || !integration || !name) { | ||
| console.error(chalk.red('Error: Missing required arguments. Use --sync, --action, or --on-event, and provide an integration and a name.')); | ||
| this.help(); | ||
| } |
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.
I feel like extracting this logic in its own file/class with functions and early returns will reduce the branching, make it easier to read and more testable as well as reusable in the different commands. Something like that:
export class Ensure {
constructor(private readonly interactive: boolean)
private async ensure<T>(
currentValue: T | undefined,
promptFn: () => Promise<T>,
errorMessage: string
): Promise<T> {
if (currentValue) return currentValue;
if (!interactive) throw new MissingArgumentError(errorMessage);
try {
return await promptFn();
catch {
// show error and process exit
}
}
public async functionType(
sync: boolean,
action: boolean,
onEvent: boolean,
): Promise<FunctionType> {
if (sync) return 'sync';
if (action) return 'action';
if (onEvent) return 'on-event';
if (!this.interactive) {
throw new MissingArgumentError('Must specify --sync, --action, or --on-event');
}
return await promptForFunctionType();
}
public async integration(
current: string | undefined,
context: { fullPath?: string; isNangoFolder: boolean; isZeroYaml:
boolean; debug?: boolean }
): Promise<string> {
return ensure(
current,
() => promptForIntegrationName(context),
'Integration name is required'
);
}
public async functionName(
current: string | undefined,
functionType: FunctionType,
): Promise<string> {
return ensure(
current,
() => promptForFunctionName(functionType),
'Function name is required'
);
}
export async function environment(
current: string | undefined,
interactive: boolean
): Promise<string> {
return ensure(
current,
interactive,
() => promptForEnvironment(),
'Environment is required'
);
}
public async function(
current: string | undefined,
availableFunctions: { name: string; type: string }[]
): Promise<string> {
return ensure(
current,
() => promptForFunctionToRun(availableFunctions),
'Function name is required'
);
}
public async connection(
current: string | undefined,
environment: string
): Promise<string> {
return ensure(
current,
() => promptForConnection(environment),
'Connection ID is required'
);
}
public async projectPath(
current: string | undefined,
): Promise<string> {
return ensure(
current,
() => promptForProjectPath(),
'Project path is required'
);
}
so it can be used like this
const ensure = new Ensure(interactive)
const functionType = await ensure.functionType(sync, action,
onEvent);
integration = await ensure.integration(integration, { ... });
name = await ensure.functionName(name, functionType);
wdyt?
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.
Great suggestion, implemented it
packages/cli/lib/index.ts
Outdated
| connectionId = await promptForConnection(environment); | ||
| } | ||
| } catch (err: any) { | ||
| console.error(chalk.red(err.isTtyError ? "Prompt couldn't be rendered in the current environment" : err.message)); |
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.
can we add some info about how users could fix the issue?
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.
Added a hint to use --no-interactive if the prompt fails (I would expect this to happen in CI/non-TTY).
packages/cli/package.json
Outdated
| "glob": "11.1.0", | ||
| "import-meta-resolve": "4.1.0", | ||
| "jscodeshift": "17.3.0", | ||
| "inquirer": "^13.1.0", |
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 always pin to a specific version. We have all collectively been burned too many time by breaking changes (even in patch version)
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.
Good call, pinned it
c5759b7 to
46b26d3
Compare
packages/cli/package.json
Outdated
| { | ||
| "name": "nango", | ||
| "version": "0.69.20", | ||
| "version": "0.70.0", |
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.
Thoughts on if this should be a minor or a patch? Sticking to strict semver its a minor, but open to change it if anyone thinks its a patch
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.
is it a breaking change? aka: does it modify the behavior of existing commands? if yes, minor otherwise patch
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.
version isn't managed by modifying the package.json directly. The release workflow takes care of it
packages/cli/lib/errors.ts
Outdated
| @@ -0,0 +1,6 @@ | |||
| export class MissingArgumentError extends Error { | |||
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 is also packages/cli/lib/utils/errors.ts. Can we unify them?
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.
Missed that file, will merge them
packages/cli/lib/index.ts
Outdated
|
|
||
| import { initAI } from './ai/init.js'; | ||
| import { generate, getVersionOutput, tscWatch } from './cli.js'; | ||
| import { MissingArgumentError } from './errors.js'; |
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.
ooc: Why do we need to import .js files here?
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.
ESM modules require file extension on imports. Addtionally, my understanding is that typescript doesn't change file extensions when transpiling to javascript, so we need to reference the js files.
Might be wrong and there is another reason why this is done, but I think this is the case
|
|
||
| // opts.interactive is true by default (from the option default), or false if --no-interactive is passed. | ||
| // We also disable it if we are in a CI environment. | ||
| opts.interactive = opts.interactive && !isCI; |
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.
When would this be used in a CI environment?
I'm not sure it's a great idea to have the completely opposite behavior of a flag in CI instead of outside of it. Would it make more sense to raise an error in CI environment and make sure people specify --no-interactive instead? This would be a breaking change, though, hence the question above :P
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.
Some customers use the CLI today on CI envs to test and deploy nango. If we force them to adopt the --no-interactive flag we would break their CI pipelines. At the same time, we want interactive mode to be the default use case for people using the CLI on the terminal, so this gets both things done. A warning for CI envs that do not provide the --no-interactive flag might be in order though. Wdyt?
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.
Yup I think a warning would be great.
46b26d3 to
fa2476d
Compare
|
|
||
| ```bash | ||
| # Command flag to auto-confirm all prompts (useful for CI). | ||
| # Note: Destructive changes (like removing a sync or renaming a model) requires confirmation, even when --auto-confirm is set. To bypass this restriction, the --allow-destructive flag can be passed to nango deploy. |
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.
[Documentation] Fix subject-verb agreement: "Destructive changes ... requires" should be "Destructive changes ... require".
Context for Agents
[**Documentation**]
Fix subject-verb agreement: "Destructive changes ... requires" should be "Destructive changes ... require".
File: docs/reference/cli.mdx
Line: 106
TBonnin
left a comment
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.
lgtm. Just the package version and dependency pinning and the rest is good to go I think
packages/cli/package.json
Outdated
| "@types/commander": "2.12.5", | ||
| "@types/ejs": "3.1.5", | ||
| "@types/figlet": "1.5.6", | ||
| "@types/inquirer": "^9.0.7", |
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.
pin, pin, pin
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.
does interactive and ensure belongs to the services folder? You are gonna ask what's a service in the context of the CLI and to be honest I am not sure I have an good answer :/
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.
I just followed suit, but it doesn't make any sense. Thought of it as just where the main logic leaves, but it is a bit a weird naming scheme if you ask me
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.
you don't need to change anything here. Let's keep it in mind though when/if we refactor the CLI ;)
2e49d99 to
e80ac29
Compare
e80ac29 to
0c471eb
Compare
…e, dry run and deploy
0c471eb to
abe63c9
Compare
This PR improves the CLI's user experience by introducing an interactive mode for the
create,dryrun,deploy, andinitcommands.
When a user runs one of these commands without providing all the required arguments, the CLI will now launch an interactive prompt to guide them through the missing options. This makes the CLI easier to use, as it no longer requires users to memorize all command-line arguments.
The interactive prompts are built using
inquirerto provide a user-friendly selection list for known values (like function types or environments).
The interactive mode is enabled by default and can be disabled with the
--no-interactiveflag or by setting theCIenvironment variable, ensuring backward compatibility and scriptability.
Fixes NAN-4237
Testing:
npm installto add the newinquirerdependency.nango createwithout any arguments. Verify that you are prompted to select a function type, integration, and provide a name.nango dryrunwithout any arguments. Verify that you are prompted to select an environment, a function, and a connection.nango deploywithout an environment. Verify that you are prompted to select one.nango initwithout a path. Verify that you are prompted to enter one.--no-interactiveflag to confirm that the prompts are skipped and the command fails due to missing arguments.A shared Ensure utility now orchestrates the interactive prompts, translating missing-argument errors into actionable guidance while preserving non-interactive execution when required.
Affected Areas
• packages/cli/lib/index.ts
• packages/cli/lib/services/interactive.service.ts
• packages/cli/lib/services/ensure.service.ts
• packages/cli/lib/services/function-create.service.ts
• packages/cli/lib/types.ts
• packages/cli/lib/services/sdk.ts
• packages/cli/lib/utils/errors.ts
• packages/cli/tsconfig.json
• packages/cli/package.json
• docs/reference/cli.mdx
• package-lock.json
This summary was automatically generated by @propel-code-bot