feat(functions): add functions:kits:install command - #10900
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
![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 the functions:kits:install command, allowing users to install Cloud Function kits into their Firebase projects. It includes implementation logic, unit tests, command registration under the kits experiment, and a TypeScript entry point template. The review feedback highlights two key areas for improvement: first, a potential runtime crash in functions-kits-install.ts due to unsafe direct access of options.config.src after using optional chaining; second, the need to throw a FirebaseError immediately if parsing the package.nolint.json template fails, rather than silently continuing with an empty object and causing downstream build failures.
2aeea99 to
e4b08d8
Compare
- throw error for failing to parse template - remove redundant ? for options.config.src, it shouldn't be undefined or null
073b681 to
2872d49
Compare
package.json, tsconfig.json, and .gitignore should live in the root of the kit, and index.ts will be in /src.
- Prompt for kit ID - Update npm shrinkwrap warning - change FIREBASE_FUNCTION_KIT_REGION param to FUNCTION_KIT_REGION
ajperel
left a comment
ajperel
left a comment
There was a problem hiding this comment.
Thanks for getting this out!
A bunch of minor comments and questions and a few things we should iron out through discussion.
| const res = parsePackageSpecifier("my-kit@^2.0.0"); | ||
| expect(res).to.deep.equal({ | ||
| packageName: "my-kit", | ||
| version: "^2.0.0", |
There was a problem hiding this comment.
Probably also good to test release candidate versions since we'll use them
There was a problem hiding this comment.
Added more tests.
| const INDEX_KIT_TEMPLATE = readTemplateSync("init/functions/typescript/index-kit.ts"); | ||
|
|
||
| export interface FunctionsKitsInstallOptions extends Options { | ||
| npm_package?: string; |
There was a problem hiding this comment.
As I see this.... I am again debating between this and
--package
(future) --package_manager
Pros:
- We only ever need 2 flags (I hope)
- Maybe easier to re-use logic in the future in cases where like... yarn packages and npm packages are the same since they are both npm behind the scenes
Cons:
- Users not using npm have to always specify two flags instead of one.
I am still very torn. Curious what you and @inlined think after getting into it more.
There was a problem hiding this comment.
I don't feel that strongly either way TBH. I preferred package and package_manager to pave the way to support other package types in the future, but I also see the argument that we're implying the utility is larger than it is.
| await options.config.askWriteProjectFile(relIndexTsPath, indexContent); | ||
| } | ||
|
|
||
| const installArgs = isThirdParty ? ["install", "--ignore-scripts"] : ["install"]; |
There was a problem hiding this comment.
I wonder if we should just always ignore scripts. Yes, we could technically trust ours more... but when would we use them?
There was a problem hiding this comment.
I'm not sure, but we can leave it as is. We can always change it if we find a reason to.
| const newKitConfig: KitFunctionConfig = { | ||
| kit: kitId, | ||
| sourcePackage: { | ||
| id: packageName, |
There was a problem hiding this comment.
This is 100% what we put in the design doc... but as I brush up while reviewing...
none of the package systems talk about ids. They talk about package / distribution names. I wonder if we should also name this name. But.... maybe not a big deal? If we did want to do it maybe easier in a separate follow up change? Thoughts.
package systems also talk about the full spec (including version, etc.) but we're not storing that and I don't think we should since it'll get out of date.
There was a problem hiding this comment.
That's a fair nit, let's keep it ID here and change it in a follow-up.
* Rename parsePackageSpecifier to parseNpmPackageSpecifier * Add validateNpmPackageName to validate npm package format and reject multiple slashes * Simplify sanitizePackageNameToKitName by removing redundant regex replacement * Tighten isThirdPartyPackage to strictly match @firebase-functions-kits/ scope * Make npm-shrinkwrap check independent of package origin and prompt if unshrinkwrapped * Fix typo in shrinkwrap warning message * Collect existing codebase and instance IDs upfront to prevent mutation side-effects during validation * Refactor validateKitInstances to accept Iterable<string> with a default Set and add JSDoc param documentation * Add rejection and tests for instance ID collisions across multiple kits * Define FUNCTION_KITS_DIR constant for directory paths * Update command and option descriptions to use function kit terminology * Add explanatory comment before configuring wrapper package.json * Refactor index-kit.ts template to use params namespace import and improve option comments * Add unit tests for package name validation, RC/tag parsing, scope spoofing, independent shrinkwrap checks, and standalone validateKitInstances
ajperel
left a comment
ajperel
left a comment
There was a problem hiding this comment.
Thanks for the fast iteration.
- Split format validation into `validateKitInstanceId` for single instance ID checking. - Renamed the collection helper to `validateAndAddKitInstances` to explicitly signal adding validated IDs to the project set. - Updated `functions:kits:install` prompt validation and tests accordingly.
* Add kits:install command * Address GCA comments - throw error for failing to parse template - remove redundant ? for options.config.src, it shouldn't be undefined or null * Update source tree package.json, tsconfig.json, and .gitignore should live in the root of the kit, and index.ts will be in /src. * Few more changes - Prompt for kit ID - Update npm shrinkwrap warning - change FIREBASE_FUNCTION_KIT_REGION param to FUNCTION_KIT_REGION * Update 3p test * feat: address review feedback for functions:kits:install * Rename parsePackageSpecifier to parseNpmPackageSpecifier * Add validateNpmPackageName to validate npm package format and reject multiple slashes * Simplify sanitizePackageNameToKitName by removing redundant regex replacement * Tighten isThirdPartyPackage to strictly match @firebase-functions-kits/ scope * Make npm-shrinkwrap check independent of package origin and prompt if unshrinkwrapped * Fix typo in shrinkwrap warning message * Collect existing codebase and instance IDs upfront to prevent mutation side-effects during validation * Refactor validateKitInstances to accept Iterable<string> with a default Set and add JSDoc param documentation * Add rejection and tests for instance ID collisions across multiple kits * Define FUNCTION_KITS_DIR constant for directory paths * Update command and option descriptions to use function kit terminology * Add explanatory comment before configuring wrapper package.json * Refactor index-kit.ts template to use params namespace import and improve option comments * Add unit tests for package name validation, RC/tag parsing, scope spoofing, independent shrinkwrap checks, and standalone validateKitInstances * Generate new kit ids defaults if taken * Update index-kit.ts template * Update prompts for 3P and adding instances * Refactor valiateKitInstances - Split format validation into `validateKitInstanceId` for single instance ID checking. - Renamed the collection helper to `validateAndAddKitInstances` to explicitly signal adding validated IDs to the project set. - Updated `functions:kits:install` prompt validation and tests accordingly. * formatting
3 participants
Description
Adds the
firebase functions:kits:installcommand (gated behind thekitsexperiment) to allow developers to install and configure reusable Cloud Function kits in their Firebase projects.Note: this is the first iteration of the command that just handles the first instance in a single project.
Key Changes:
functions:kits:installconditionally when thekitsexperiment is enabled.--npm_package <package>flag, and interactively prompts for the kit and initial instance ID.firebase.json.@firebase-functions-kits/*) and third-party packages.npm-shrinkwrap.jsonvianpm pack --dry-run --jsonto warn users when dependencies are unlocked.--ignore-scriptsfor safety.function-kits/<kit-id>/containingpackage.json,tsconfig.json,.gitignore, andsrc/index.tsfrom templates.index.tsusingtemplates/init/functions/typescript/index-kit.ts, re-exporting the kit package with customizable global function parameters.npm installandnpm run build.firebase.json.src/commands/functions-kits-install.spec.ts.Scenarios Tested
kitsexperiment is disabled.firebase.jsonmissing).npm-shrinkwrap.json.npm install --ignore-scripts.package.json,tsconfig.json,index.ts,firebase.json).functions.kitID infirebase.json.instanceIdand existing functioncodebasename.Sample Commands