functions:kits:list - #10935
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request introduces the functions:kits:list command to list installed kits from firebase.json when the 'kits' experiment is enabled. The feedback recommends improving robustness by handling scenarios where the command is run outside of a Firebase project directory (checking for options.config and throwing a FirebaseError) and gracefully returning an empty array if no functions configuration is present in firebase.json.
Sorry, something went wrong.
| import { Command } from "../command"; | ||
| import { listKitConfigs } from "../functions/kits/config"; | ||
| import { Options } from "../options"; | ||
| import { logLabeledBullet } from "../utils"; | ||
| import { logger } from "../logger"; | ||
| import * as Table from "cli-table3"; |
There was a problem hiding this comment.
Import FirebaseError to handle the case where the command is run outside of a Firebase project directory.
| import { Command } from "../command"; | |
| import { listKitConfigs } from "../functions/kits/config"; | |
| import { Options } from "../options"; | |
| import { logLabeledBullet } from "../utils"; | |
| import { logger } from "../logger"; | |
| import * as Table from "cli-table3"; | |
| import { Command } from "../command"; | |
| import { listKitConfigs } from "../functions/kits/config"; | |
| import { Options } from "../options"; | |
| import { logLabeledBullet } from "../utils"; | |
| import { logger } from "../logger"; | |
| import * as Table from "cli-table3"; | |
| import { FirebaseError } from "../error"; |
Sorry, something went wrong.
| .action((options: Options) => { | ||
| const firebaseConfig = options.config; | ||
| const validatedConfig = firebaseConfig.src; | ||
| const kitConfigs = listKitConfigs(validatedConfig); |
There was a problem hiding this comment.
If the command is run outside of a Firebase project directory, options.config will be undefined. Accessing options.config.src will throw a TypeError. We should explicitly check if options.config is defined and throw a FirebaseError if it is not, adhering to the repository style guide on strict null checks and throwing user-facing errors.
| .action((options: Options) => { | |
| const firebaseConfig = options.config; | |
| const validatedConfig = firebaseConfig.src; | |
| const kitConfigs = listKitConfigs(validatedConfig); | |
| .action((options: Options) => { | |
| const firebaseConfig = options.config; | |
| if (!firebaseConfig) { | |
| throw new FirebaseError( | |
| "No active project configuration found. Please run this command from within a Firebase project directory." | |
| ); | |
| } | |
| const validatedConfig = firebaseConfig.src; | |
| const kitConfigs = listKitConfigs(validatedConfig); |
- Use strict null checks and handle undefined/null explicitly. Throw FirebaseError for expected, user-facing errors. (link)
Sorry, something went wrong.
| export function listKitConfigs(config: FirebaseConfig): ValidatedKitSingle[] { | ||
| const normalized = normalizeAndValidate(config.functions); | ||
| return normalized.filter((s) => isKitConfig(s)); | ||
| } |
There was a problem hiding this comment.
If firebase.json does not contain a functions section, config.functions will be undefined. Calling normalizeAndValidate(undefined) will throw a FirebaseError stating "No valid functions configuration detected in firebase.json". For a list command, it is better to gracefully return an empty array so the command can print "there are no kits in firebase.json" instead of throwing an error.
| export function listKitConfigs(config: FirebaseConfig): ValidatedKitSingle[] { | |
| const normalized = normalizeAndValidate(config.functions); | |
| return normalized.filter((s) => isKitConfig(s)); | |
| } | |
| export function listKitConfigs(config: FirebaseConfig): ValidatedKitSingle[] { | |
| if (!config.functions) { | |
| return []; | |
| } | |
| const normalized = normalizeAndValidate(config.functions); | |
| return normalized.filter((s) => isKitConfig(s)); | |
| } |
- Use strict null checks and handle undefined/null explicitly. (link)
Sorry, something went wrong.
inlined
left a comment
There was a problem hiding this comment.
Fix errors obviously.
Looking at src/commands/deploy.ts it looks like the standard way of ensuring there's a firebase config is .before(requireConfig)
Sorry, something went wrong.
wandamora
left a comment
There was a problem hiding this comment.
Agree with Thomas. LGTM % gemini feedback.
Sorry, something went wrong.
No description provided.