Skip to content

fix: explicitly install packages with --save-prefix=^ - #10980

Merged
wandamora merged 1 commit into
mainfrom
morawand-fix-kits-install-semver
Aug 24, 2026
Merged

fix: explicitly install packages with --save-prefix=^#10980
wandamora merged 1 commit into
mainfrom
morawand-fix-kits-install-semver

Conversation

@wandamora

Copy link
Copy Markdown
Contributor

Description

This will make sure that npm packages we install for kits can accept any non-major version update.

Scenarios Tested

  • firebase functions:kits:install --package <package>@0.1.0 installs and updates package.json with "^0.1.0" version.
  • firebase functions:kits:install --package <package>@latest installs and updates package.json with "^0.1.0" version.

This will make sure that npm packages we install for kits can accept
any non-major version update.

@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 updates the functions:kits:install command to install the specified kit package directly via npm install <package-name> --save-prefix=^ instead of manually writing the dependency into the scaffolded package.json and running a bare npm install. Consequently, the version parameter is removed from several helper functions like writeKitPackageJson and scaffoldKitFiles, and the corresponding unit tests have been updated to reflect these changes. I have no feedback to provide as there are no review comments.

@wandamora
wandamora force-pushed the morawand-fix-kits-install-semver branch from 7ac1386 to dbb79f9 Compare August 24, 2026 21:07
@wandamora
wandamora marked this pull request as ready for review August 24, 2026 21:07

@ajperel ajperel 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.

Two reasonable comments from the tools repo review skill as well.

*/
export async function buildAndInstallKit(
absSourcePath: string,
rawPkgName: string,

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.

🟡 [Component Design] Options Object for buildAndInstallKit\n\nRationale: The function now accepts 3 positional parameters. According to our design standards (Category B, Rule 1), functions accepting multiple arguments should accept a single options object to make call sites self-documenting and future-proof.\n\nSuggested Fix:\nts\nexport interface BuildAndInstallKitOptions {\n absSourcePath: string;\n rawPkgName: string;\n isThirdParty: boolean;\n}\n\nexport async function buildAndInstallKit(options: BuildAndInstallKitOptions): Promise<void> {\n const { absSourcePath, rawPkgName, isThirdParty } = options;\n const installArgs = [\"install\", rawPkgName, \"--save-prefix=^\"];\n if (isThirdParty) {\n installArgs.push(\"--ignore-scripts\");\n }\n logLabeledBullet(\"functions\", `Running npm ${installArgs.join(\" \")}...`);\n // ... rest of the function\n}\n\n\nDon't forget to update the call site in src/commands/functions-kits-install.ts and the tests in src/commands/functions-kits-install.spec.ts if you apply this.

@@ -539,7 +535,6 @@ export async function scaffoldKitFiles(
kitId: string,
instanceId: string,
packageName: string,

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.

🟡 Nit: [Component Design] Options Object for scaffoldKitFiles\n\nRationale: Although we reduced the parameter count from 5 to 4 in this PR, it still has many positional parameters. Consider refactoring it to use an options object as well.\n\nSuggested Fix:\nts\nexport interface ScaffoldKitFilesOptions {\n config: Config;\n kitId: string;\n instanceId: string;\n packageName: string;\n templateType?: TemplateType;\n}\n\nexport async function scaffoldKitFiles(options: ScaffoldKitFilesOptions): Promise<ScaffoldedKitPaths> {\n const { config, kitId, instanceId, packageName, templateType = DEFAULT_TEMPLATE } = options;\n // ...\n}\n

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have a larger refactor PR in flight and it refactors a lot of the calls to use an options object as requested here. I'll save it for that PR: #10967

@wandamora
wandamora merged commit 3ecd534 into main Aug 24, 2026
52 of 53 checks passed
@wandamora
wandamora deleted the morawand-fix-kits-install-semver branch August 24, 2026 22:43
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.

3 participants