Skip to content

functions:kits:list - #10935

Open
Berlioz wants to merge 11 commits into
mainfrom
vsfan_scratch
Open

functions:kits:list#10935
Berlioz wants to merge 11 commits into
mainfrom
vsfan_scratch

Conversation

@Berlioz

@Berlioz Berlioz commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +1 to +6
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";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Import FirebaseError to handle the case where the command is run outside of a Firebase project directory.

Suggested change
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";

Comment on lines +11 to +14
.action((options: Options) => {
const firebaseConfig = options.config;
const validatedConfig = firebaseConfig.src;
const kitConfigs = listKitConfigs(validatedConfig);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Suggested change
.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);
References
  1. Use strict null checks and handle undefined/null explicitly. Throw FirebaseError for expected, user-facing errors. (link)

Comment on lines +7 to +10
export function listKitConfigs(config: FirebaseConfig): ValidatedKitSingle[] {
const normalized = normalizeAndValidate(config.functions);
return normalized.filter((s) => isKitConfig(s));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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));
}
References
  1. Use strict null checks and handle undefined/null explicitly. (link)

@inlined inlined left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@wandamora wandamora left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Thomas. LGTM % gemini feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants