feat: implement progressive CLI help disclosure and dynamic deploy help#10772
feat: implement progressive CLI help disclosure and dynamic deploy help#10772joehan wants to merge 11 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces progressive help for CLI commands, dynamically registering intermediate namespaces and filtering subcommands. It also refactors deploy targets to centralize their metadata, enabling dynamic help generation and detailed target-specific help via firebase help deploy:<target>. The review feedback highlights several opportunities to align with the repository style guide, such as replacing console.log with the central logger, avoiding any type escapes with proper interfaces, correcting an inaccurate reference to database indexes, and improving type safety in dynamic imports to prevent potential prototype pollution.
|
Could you provide some example about how CLI help looks like now? It is a bit hard to comprehend by reading your code. Please provide some CLI example and their output. Thank you |
Done - added a bunch of example output to the description. |
| "Configuration format in firebase.json:\n" + | ||
| "{\n" + | ||
| ' "extensions": {\n' + | ||
| ' "storage-resize-images": "extensions/storage-resize-images.env"\n' + |
There was a problem hiding this comment.
This is wrong. The expected format is "my-extension-instance-id": "firebase/storage-resize-images@^0.1.0"
…ord deploy help text
…te setup from command runner
| "\nTo deploy specific targets, use `--only` followed by a comma-separated list of targets.\n"; | ||
| targetHelp += `For example, ${boldFn("firebase deploy --only hosting,firestore")}.\n`; | ||
| targetHelp += `\nTo see detailed setup and configuration details for a specific target, run:\n`; | ||
| targetHelp += ` ${boldFn("firebase help deploy:<target>")}`; |
There was a problem hiding this comment.
What's the preferred way to get help, firebase help <cmd> or firebase <cmd> --help? This suggests the former but src/bin/progressiveHelp.ts suggests the latter. It's okay to support both but we should only suggest one consistently to avoid confusion
| .before(requireConfig) | ||
| .before((options: Options) => { | ||
| options.filteredTargets = filterTargets(options, VALID_DEPLOY_TARGETS); | ||
| options.filteredTargets = filterTargets(options, [...VALID_DEPLOY_TARGETS]); |
| const client = this.client; // eslint-disable-line @typescript-eslint/no-invalid-this | ||
| if (commandName && commandName.startsWith("deploy:")) { | ||
| const targetName = commandName.split(":")[1]; | ||
| const { TARGETS, VALID_DEPLOY_TARGETS } = require("../deploy") as typeof import("../deploy"); |
There was a problem hiding this comment.
Should this use src/dynamicImport.js? I'm not sure either
| targetHelp += | ||
| "\nTo deploy specific targets, use `--only` followed by a comma-separated list of targets.\n"; | ||
| targetHelp += `For example, ${boldFn("firebase deploy --only hosting,firestore")}.\n`; | ||
| targetHelp += `\nTo see detailed setup and configuration details for a specific target, run:\n`; |
There was a problem hiding this comment.
nit: repetitive use of the word "details"
| const target = isValidTarget ? TARGETS[targetName as keyof typeof TARGETS] : undefined; | ||
| if (target && target.detailedHelp) { | ||
| logger.info(); | ||
| logger.info(clc.bold(`Detailed deploy information for ${targetName}:`)); |
There was a problem hiding this comment.
nit: I find the phrase "deploy information" a bit confusing. It almost sounds like information of my deployment in production. Do we need this header? Or can this be phrased differently e.g. "detailed setup and configuration" to be consistent with firebase deploy --help?
| return; | ||
| } else { | ||
| logger.warn(); | ||
| utils.logWarning(`No detailed deploy information found for target: ${targetName}`); |
There was a problem hiding this comment.
I find this line confusing too TBH. And have we considered just delegating to firebase deploy --help text? Saying "just run another command" doesn't seem that useful to me. Humans will need to do additional copy and paste, agents waste context window and may prompt the human to approve yet another command. This gets especially frustrating after following the advice from firebase deploy --help only to be pointed back to where they came from.
What if we just say:
See Firebase documentation on deploying <targetName>.
For example, you can find a full reference of `firebase.json` on https://firebase.google.com/docs/cli#the_firebasejson_file"
Here's some general information about the `firebase deploy` command just in case:
<delegated output here>
|
|
||
| export { prepare, deploy, release }; | ||
|
|
||
| export const help = "Deploys security rules and indexes defined in your project's firebase.json."; |
There was a problem hiding this comment.
| export const help = "Deploys security rules and indexes defined in your project's firebase.json."; | |
| export const help = "Deploys security rules and indexes referenced by your project's firebase.json."; |
| export { deploy } from "./deploy"; | ||
| export { release } from "./release"; | ||
|
|
||
| export const help = "Deploys security rules defined in your project's firebase.json."; |
There was a problem hiding this comment.
| export const help = "Deploys security rules defined in your project's firebase.json."; | |
| export const help = "Deploys security rules referenced by your project's firebase.json."; |
| "{\n" + | ||
| ' "database": [\n' + | ||
| ' { "target": "my-db-target", "rules": "rules.rules" },\n' + | ||
| ' { "database": "my-database-id", "rules": "rules.rules" }\n' + |
There was a problem hiding this comment.
| ' { "database": "my-database-id", "rules": "rules.rules" }\n' + | |
| ' { "database": "my-database-id", "rules": "rules2.rules" }\n' + |
4 participants
Description
Revamping the --help and firebase help command to use progressive disclosure. Instead of dumping a massive list of commands all at once, you can navigate this layer by layer so that there is not a ton of irrelevant info flooding the terminal (or context).
I also cleaned up some of our Winston logging transport initialization logic - it was spread our over a few places (and was happening late enough to miss some errors early in the flow). Now, it lives at the main entry point for the cli binary.
I also ran our eval suite before and after this change - in cases where the --help command was run, we saw significant performance improvements:
Averages Across Help-Executing Cases
Testing
Before this change,
firebase --helpwould print every single command, andfirebase <command> helpwould print the help text for just that command. There was no support forfirebase <namespace> --help- doing so would just give you a 'command not found error'Some examples of the new behavior - at the top level,
--helpjust returns all of the top level namespaces:To dig deeper you request help on the hosting namespace. It prints only the hosting subcommands and sub-namespaces (like channel and sites), filtering out all other unrelated CLI commands:
This applies for all namespace levels - for example, running help on
hosting:channellists all leaf commands scoped strictly under that namespace:I also improved the flow for deploy - that is one of our most complicated comands, and it previously offered no meaningful help:
Now, it offers details about all the targets:
And, you can view specific details about how to use a target: