Repository navigation
fix(functions): run the SDK bin entry when no .bin shim exists - #11387
abhiramshaji-cyber wants to merge 8 commits into
Conversation
Deploy looked for an extensionless node_modules/.bin/firebase-functions file and gave up when it was missing. Bun on Windows only writes firebase-functions.exe and firebase-functions.bunx, so every deploy stopped with "Failed to find location of Firebase Functions SDK" even though the SDK itself resolved fine. The .bin lookup stays first so working setups are unchanged. When it finds nothing, the CLI now reads the bin entry the SDK declares in its own package.json and runs it with node, which is what the shim would have done. This does not depend on how a package manager names its shims or where it stores packages. Fixes firebase#11386
![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 addresses an issue where firebase deploy fails to locate the Cloud Functions SDK under certain package managers (like Bun on Windows or PNPM) that do not create a standard node_modules/.bin/firebase-functions shim. It introduces a helper to locate the SDK's binary entry point via its package.json and updates the process spawning logic to execute it with Node. The feedback recommends enhancing robustness by wrapping the package.json parsing in a try-catch block and using the class's existing this.bin property instead of hardcoding the global 'node' binary, along with corresponding updates to the test assertions.
| function findSdkBinEntry(sdkPath: string): string | undefined { | ||
| for ( | ||
| let dir = path.dirname(sdkPath); | ||
| path.basename(dir) !== "node_modules" && path.dirname(dir) !== dir; | ||
| dir = path.dirname(dir) | ||
| ) { | ||
| const packageJsonPath = path.join(dir, "package.json"); | ||
| if (!fileExistsSync(packageJsonPath)) { | ||
| continue; | ||
| } | ||
| const { name, bin } = JSON.parse(fs.readFileSync(packageJsonPath, "utf8")) as { | ||
| name?: string; | ||
| bin?: string | Record<string, string>; | ||
| }; | ||
| if (name !== "firebase-functions") { | ||
| continue; | ||
| } | ||
| const entry = typeof bin === "string" ? bin : bin?.["firebase-functions"]; | ||
| return entry ? path.join(dir, entry) : undefined; | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
To ensure robustness and adhere to defensive programming practices, we should wrap the JSON parsing and file reading in a try-catch block. Additionally, we should explicitly validate that the parsed package.json is a non-null object and that the resolved 'bin' entry is a string before attempting to join paths. This prevents potential unhandled exceptions or crashes if a package.json is malformed or empty.
function findSdkBinEntry(sdkPath: string): string | undefined {
for (
let dir = path.dirname(sdkPath);
path.basename(dir) !== "node_modules" && path.dirname(dir) !== dir;
dir = path.dirname(dir)
) {
const packageJsonPath = path.join(dir, "package.json");
if (!fileExistsSync(packageJsonPath)) {
continue;
}
try {
const content = fs.readFileSync(packageJsonPath, "utf8");
const parsed = JSON.parse(content);
if (!parsed || typeof parsed !== "object") {
continue;
}
const { name, bin } = parsed as {
name?: string;
bin?: string | Record<string, string>;
};
if (name !== "firebase-functions") {
continue;
}
const entry = typeof bin === "string" ? bin : (bin && typeof bin === "object" ? bin["firebase-functions"] : undefined);
if (typeof entry === "string") {
return path.join(dir, entry);
}
} catch (err) {
logger.debug("Failed to parse package.json at " + packageJsonPath + ":", err);
}
}
return undefined;
}References
- Avoid using any or unknown as an escape hatch, and use strict null checks and handle undefined/null explicitly. (link)
There was a problem hiding this comment.
Thanks. Partly applied in 6e136d7: the bin entry is now only used when it is a string, so an odd shape falls through to the existing error instead of a TypeError in path.join. I left out the try/catch: the package.json being read is the one Node already resolved main from, so it parses, and swallowing a parse error would hide the real cause behind the generic "Failed to find location" message.
| const binEntry = findSdkBinEntry(sdkPath); | ||
| if (binEntry) { | ||
| logger.debug(`Found firebase-functions bin entry at '${binEntry}'`); | ||
| return ["node", binEntry]; | ||
| } |
There was a problem hiding this comment.
Instead of hardcoding the global 'node' binary from the PATH, we should use 'this.bin' (which resolves to the correct Node.js binary version/path via 'getNodeBinary()'). This ensures consistency with the rest of the class and respects the requested runtime version, local cache, or standalone CLI bundle.
| const binEntry = findSdkBinEntry(sdkPath); | |
| if (binEntry) { | |
| logger.debug(`Found firebase-functions bin entry at '${binEntry}'`); | |
| return ["node", binEntry]; | |
| } | |
| const binEntry = findSdkBinEntry(sdkPath); | |
| if (binEntry) { | |
| logger.debug("Found firebase-functions bin entry at '" + binEntry + "'"); | |
| return [this.bin, binEntry]; | |
| } |
References
- Reuse or extend existing utilities and helpers rather than writing bespoke inline logic. (link)
There was a problem hiding this comment.
Good catch, done in 6e136d7. this.bin resolves to process.execPath or the local node_modules/node, which is also what the emulator spawns functions with, and it no longer depends on node being on PATH (not guaranteed for Bun only setups on Windows).
| function findFunctionsBinary(): string[] { | ||
| const delegate = new node.Delegate(PROJECT_ID, projectDir, sourceDir, "nodejs24"); | ||
| return delegate["findFunctionsBinary"](); | ||
| } |
There was a problem hiding this comment.
To support asserting against the dynamically resolved Node binary ('this.bin') instead of a hardcoded 'node', let's update the helper to return both the command array and the resolved binary path.
function findFunctionsBinary(): { binary: string[]; bin: string } {
const delegate = new node.Delegate(PROJECT_ID, projectDir, sourceDir, "nodejs24");
return {
binary: delegate["findFunctionsBinary"](),
bin: delegate.bin,
};
}| it("prefers the node_modules/.bin shim when it exists", () => { | ||
| installSdk("", ["firebase-functions"], { | ||
| "firebase-functions": "lib/bin/firebase-functions.js", | ||
| }); | ||
| expect(findFunctionsBinary()).to.deep.equal([ | ||
| path.join(sourceDir, "node_modules", ".bin", "firebase-functions"), | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Update the assertion to destructure the 'binary' property from the updated helper.
| it("prefers the node_modules/.bin shim when it exists", () => { | |
| installSdk("", ["firebase-functions"], { | |
| "firebase-functions": "lib/bin/firebase-functions.js", | |
| }); | |
| expect(findFunctionsBinary()).to.deep.equal([ | |
| path.join(sourceDir, "node_modules", ".bin", "firebase-functions"), | |
| ]); | |
| }); | |
| it("prefers the node_modules/.bin shim when it exists", () => { | |
| installSdk("", ["firebase-functions"], { | |
| "firebase-functions": "lib/bin/firebase-functions.js", | |
| }); | |
| const { binary } = findFunctionsBinary(); | |
| expect(binary).to.deep.equal([ | |
| path.join(sourceDir, "node_modules", ".bin", "firebase-functions"), | |
| ]); | |
| }); |
| it("runs the SDK bin entry with node when Bun on Windows only creates .exe shims", () => { | ||
| const sdkDir = installSdk( | ||
| path.join(".bun", "firebase-functions@7.4.0+abc", "node_modules"), | ||
| ["firebase-functions.exe", "firebase-functions.bunx"], | ||
| { "firebase-functions": "lib/bin/firebase-functions.js" }, | ||
| ); | ||
| expect(findFunctionsBinary()).to.deep.equal([ | ||
| "node", | ||
| path.join(sdkDir, "lib", "bin", "firebase-functions.js"), | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Update the assertion to use the dynamically resolved Node binary path from the helper.
| it("runs the SDK bin entry with node when Bun on Windows only creates .exe shims", () => { | |
| const sdkDir = installSdk( | |
| path.join(".bun", "firebase-functions@7.4.0+abc", "node_modules"), | |
| ["firebase-functions.exe", "firebase-functions.bunx"], | |
| { "firebase-functions": "lib/bin/firebase-functions.js" }, | |
| ); | |
| expect(findFunctionsBinary()).to.deep.equal([ | |
| "node", | |
| path.join(sdkDir, "lib", "bin", "firebase-functions.js"), | |
| ]); | |
| }); | |
| it("runs the SDK bin entry with node when Bun on Windows only creates .exe shims", () => { | |
| const sdkDir = installSdk( | |
| path.join(".bun", "firebase-functions@7.4.0+abc", "node_modules"), | |
| ["firebase-functions.exe", "firebase-functions.bunx"], | |
| { "firebase-functions": "lib/bin/firebase-functions.js" }, | |
| ); | |
| const { binary, bin } = findFunctionsBinary(); | |
| expect(binary).to.deep.equal([ | |
| bin, | |
| path.join(sdkDir, "lib", "bin", "firebase-functions.js"), | |
| ]); | |
| }); |
| it("runs the SDK bin entry with node when a .pnpm store only has .cmd shims", () => { | ||
| const sdkDir = installSdk( | ||
| path.join(".pnpm", "firebase-functions@7.4.0", "node_modules"), | ||
| ["firebase-functions.cmd", "firebase-functions.ps1"], | ||
| { "firebase-functions": "lib/bin/firebase-functions.js" }, | ||
| ); | ||
| expect(findFunctionsBinary()).to.deep.equal([ | ||
| "node", | ||
| path.join(sdkDir, "lib", "bin", "firebase-functions.js"), | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Update the assertion to use the dynamically resolved Node binary path from the helper.
| it("runs the SDK bin entry with node when a .pnpm store only has .cmd shims", () => { | |
| const sdkDir = installSdk( | |
| path.join(".pnpm", "firebase-functions@7.4.0", "node_modules"), | |
| ["firebase-functions.cmd", "firebase-functions.ps1"], | |
| { "firebase-functions": "lib/bin/firebase-functions.js" }, | |
| ); | |
| expect(findFunctionsBinary()).to.deep.equal([ | |
| "node", | |
| path.join(sdkDir, "lib", "bin", "firebase-functions.js"), | |
| ]); | |
| }); | |
| it("runs the SDK bin entry with node when a .pnpm store only has .cmd shims", () => { | |
| const sdkDir = installSdk( | |
| path.join(".pnpm", "firebase-functions@7.4.0", "node_modules"), | |
| ["firebase-functions.cmd", "firebase-functions.ps1"], | |
| { "firebase-functions": "lib/bin/firebase-functions.js" }, | |
| ); | |
| const { binary, bin } = findFunctionsBinary(); | |
| expect(binary).to.deep.equal([ | |
| bin, | |
| path.join(sdkDir, "lib", "bin", "firebase-functions.js"), | |
| ]); | |
| }); |
| it("accepts a bin entry declared as a single path", () => { | ||
| const sdkDir = installSdk("", [], "lib/bin/firebase-functions.js"); | ||
| expect(findFunctionsBinary()).to.deep.equal([ | ||
| "node", | ||
| path.join(sdkDir, "lib", "bin", "firebase-functions.js"), | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Update the assertion to use the dynamically resolved Node binary path from the helper.
| it("accepts a bin entry declared as a single path", () => { | |
| const sdkDir = installSdk("", [], "lib/bin/firebase-functions.js"); | |
| expect(findFunctionsBinary()).to.deep.equal([ | |
| "node", | |
| path.join(sdkDir, "lib", "bin", "firebase-functions.js"), | |
| ]); | |
| }); | |
| it("accepts a bin entry declared as a single path", () => { | |
| const sdkDir = installSdk("", [], "lib/bin/firebase-functions.js"); | |
| const { binary, bin } = findFunctionsBinary(); | |
| expect(binary).to.deep.equal([ | |
| bin, | |
| path.join(sdkDir, "lib", "bin", "firebase-functions.js"), | |
| ]); | |
| }); |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11387 +/- ##
=======================================
Coverage ? 59.32%
=======================================
Files ? 859
Lines ? 50155
Branches ? 10027
=======================================
Hits ? 29753
Misses ? 18116
Partials ? 2286 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Use this.bin instead of the node on PATH, and ignore a bin entry that is not a string.
|
Pushed 6e136d7 addressing the review: the fallback now runs the SDK bin entry with @IzaakGough thank you for reproducing this on Windows. Whenever you have a moment, I would really appreciate it if you could try this branch there, since I could only exercise the Windows layouts through the specs. |
|
Hi there, hope things are going well on your end! Thanks so much for your time. When someone gets a chance, could the CI Tests workflow be approved here and on #11385? Happy to change anything you'd prefer. |
|
/joe-review |
| if (!fileExistsSync(packageJsonPath)) { | ||
| continue; | ||
| } | ||
| const { name, bin } = JSON.parse(fs.readFileSync(packageJsonPath, "utf8")) as { |
There was a problem hiding this comment.
🔴 [Robustness & Error Resilience] Guard against malformed or unreadable package.json
Rationale:
src/deploy/functions/runtimes/node/index.ts#L66-L69
fs.readFileSync and JSON.parse are executed synchronously without exception handling. If an ancestor directory contains a malformed or empty package.json, or if filesystem permissions prevent reading it, JSON.parse will throw an unhandled SyntaxError (or fs error) and terminate the entire deploy command unexpectedly.
Suggested Fix:
Wrap the read and parse in a try ... catch block so any read/parse failure logs a debug trace and allows directory traversal to continue cleanly:
let name: string | undefined;
let bin: string | Record<string, string> | undefined;
try {
const parsed = JSON.parse(fs.readFileSync(packageJsonPath, "utf8")) as {
name?: string;
bin?: string | Record<string, string>;
};
name = parsed.name;
bin = parsed.bin;
} catch (err: any) {
logger.debug(`Failed to parse ${packageJsonPath}: ${err.message}`);
continue;
}There was a problem hiding this comment.
Thanks @joehan, that makes sense, especially for an unrelated package.json higher up the tree. Done in 27cf57e: the read and parse are wrapped in a try/catch that logs at debug level and keeps walking up. I also added a test where a malformed package.json sits between the entry point and the SDK root.
| continue; | ||
| } | ||
| const entry = typeof bin === "string" ? bin : bin?.["firebase-functions"]; | ||
| return typeof entry === "string" && entry ? path.join(dir, entry) : undefined; |
There was a problem hiding this comment.
🟡 [Defensive Verification] Verify resolved binary entry exists on disk
Rationale:
src/deploy/functions/runtimes/node/index.ts#L73-L74
If package.json declares a bin entry (e.g. "lib/bin/firebase-functions.js") but the referenced target does not actually exist on disk (for instance, if the SDK was incompletely installed or unbuilt), findSdkBinEntry will still return the path. When spawnFunctionsProcess spawns this.bin with that missing path, Node will exit with an unhelpful MODULE_NOT_FOUND error. If we verify that the file exists before returning, missing entries will cleanly fall through to the structured FirebaseError("Failed to find location of Firebase Functions SDK...") on line 224.
Suggested Fix:
const entry = typeof bin === "string" ? bin : bin?.["firebase-functions"];
if (typeof entry === "string" && entry) {
const resolved = path.join(dir, entry);
if (fileExistsSync(resolved)) {
return resolved;
}
}
return undefined;There was a problem hiding this comment.
Good call, an unbuilt or half installed SDK would have failed with a pretty confusing MODULE_NOT_FOUND. Done in 27cf57e: the bin path is only returned when the file exists (using fileExistsSync), so a missing entry now falls through to the existing FirebaseError. Added a test for it too.
| } | ||
|
|
||
| function findFunctionsBinary(): { binary: string[]; bin: string } { | ||
| const delegate = new node.Delegate(PROJECT_ID, projectDir, sourceDir, "nodejs24"); |
There was a problem hiding this comment.
🟡 Nit: [Testing Discipline] Testing private helper via index bracket notation
Rationale:
src/deploy/functions/runtimes/node/index.spec.ts#L164-L166
Reaching into the private method via delegate["findFunctionsBinary"]() bypasses TypeScript's type visibility controls. While pragmatic here because spawnFunctionsProcess initiates a child process, exporting findSdkBinEntry directly with an @internal JSDoc annotation would provide a clean, typed unit test seam without needing index notation on Delegate.
There was a problem hiding this comment.
Thanks, agreed it reads much cleaner. In 27cf57e findSdkBinEntry is exported with @internal and the bin entry cases (Bun store, .pnpm store, string bin, no bin, missing file, malformed package.json) now test it directly. I kept three small findFunctionsBinary tests for the parts that live on the Delegate: the .bin shim winning, pairing the entry with this.bin, and the error when nothing is found. Would you rather I drop those so there is no bracket access at all? Happy either way.
…n entry to exist Export findSdkBinEntry and test it directly, covering a malformed package.json and a bin entry that points to a missing file.
…ctions-binary # Conflicts: # CHANGELOG.md
…ctions-binary # Conflicts: # CHANGELOG.md
|
Hi @joehan, hope you're doing well! |
…ctions-binary # Conflicts: # CHANGELOG.md
|
Hi @joehan, hope your week is going well! |
…ctions-binary # Conflicts: # CHANGELOG.md
Description
Deploying functions on Windows with Bun stopped because the CLI could not find the Functions SDK, and now it runs the SDK's own declared entry file instead.
What this code is for
findFunctionsBinary()insrc/deploy/functions/runtimes/node/index.tsfinds the program inside thefirebase-functionspackage that reads a codebase and reports its functions.spawnFunctionsProcess()runs that program duringfirebase deployand in the emulator, and the CLI builds the deploy plan from what it reports.Before this PR
A developer on Windows installs dependencies with
bun installand runsfirebase deploy --only functions, as @philemonkipngetich described in #11386.spawnFunctionsProcess()callsfindFunctionsBinary().require.resolve("firebase-functions", { paths: [this.sourceDir] })succeeds and points into Bun's store,node_modules/.bun/firebase-functions@7.4.0+<hash>/node_modules/firebase-functions/lib/v2/index.js.node_modulesfolders for a file named exactly.bin/firebase-functions..bin/firebase-functions.exeand.bin/firebase-functions.bunx, sofileExistsSync()is false in all four folders.Failed to find location of Firebase Functions SDKand the deploy stops.After this PR
Same developer, same command. Same as above through step 4.
findSdkBinEntry(sdkPath)walks up from the resolved file to thepackage.jsonwhosenameisfirebase-functions.binfield,{ "firebase-functions": "lib/bin/firebase-functions.js" }, joins it to that folder, and checks the file is really there.findFunctionsBinary()returns[this.bin, "<sdk folder>/lib/bin/firebase-functions.js"], wherethis.binis the node binary the CLI already resolved.spawnFunctionsProcess()runs that node with<that file> <sourceDir>, the same program the.binfile would have run, and the deploy continues.A developer using npm still has
.bin/firebase-functions, so step 3 finds it and nothing else runs.The diagram
flowchart LR classDef box fill:#eef1ff,stroke:#4c5bd4,color:#111827 classDef bad fill:#ffd6d6,stroke:#cc0000,color:#111827,stroke-width:3px classDef good fill:#c8f0c8,stroke:#008800,color:#111827,stroke-width:3px subgraph BEFORE["BEFORE: deploy stops with an error"] direction TB B1["firebase deploy on Windows with Bun"] --> B2["findFunctionsBinary()"] B2 --> B3["check 4 node_modules/.bin folders"] B3 --> B4{"file named .bin/firebase-functions?"} B4 -->|"only .exe and .bunx exist"| B5["Failed to find location of Firebase Functions SDK"] B4 -->|"npm, file found"| B6["spawn the .bin file"] end subgraph AFTER["AFTER: deploy continues"] direction TB A1["firebase deploy on Windows with Bun"] --> A2["findFunctionsBinary()"] A2 --> A3["check 4 node_modules/.bin folders"] A3 --> A4{"THE CHANGE: no file, read bin from SDK package.json"} A4 -->|"only .exe and .bunx exist"| A5["spawn node lib/bin/firebase-functions.js"] A4 -->|"npm, file found"| A6["spawn the .bin file, as today"] end B1 ~~~ A1 class B1,B2,B3,B6 box class A1,A2,A3,A6 box class B4,B5 bad class A4,A5 good linkStyle 3 stroke:#cc0000,stroke-width:5px linkStyle 8 stroke:#008800,stroke-width:5px style BEFORE fill:#fff5f5,stroke:#cc0000,color:#111827 style AFTER fill:#f3fff3,stroke:#008800,color:#111827The change
.binloop finds nothing, a new functionfindSdkBinEntry(sdkPath)reads thebinfield from the SDK's ownpackage.json. It accepts both forms npm allows, a single path string or an object keyed byfirebase-functions. If there is an entry and the file exists,findFunctionsBinary()returns[this.bin, entry]; otherwise the sameFirebaseErroras before is thrown, instead of a laterMODULE_NOT_FOUND.findFunctionsBinary()now returns the command and its leading arguments as an array, andspawnFunctionsProcess()spreads it intospawn(command, [...args, this.sourceDir]).package.jsonby walking up from the resolved file and checking itsname, becausefirebase-functionsdoes not list./package.jsonin itsexports, sorequire.resolve("firebase-functions/package.json")throws..binlookup first so setups that work today run exactly the same command. I would really value your view on the order: would you prefer thepackage.jsonentry to be tried first, or kept as the fallback like this?nodeisthis.bin, which comes fromgetNodeBinary()(the localnode_modules/nodeorprocess.execPath), so it does not depend onnodebeing onPATH. Readingthis.binalso logsUsing node@N from host.(or the version mismatch warning) during deploy, and only on this fallback path; setups that find the.binfile see no new log line.package.jsonon the way up that cannot be read or parsed is logged at debug level and skipped, so it never stops the deploy.findSdkBinEntryis exported with@internalso the spec tests it directly.CHANGELOG.mdline.What changes for a user
node_modules/.bunstore..bin/firebase-functionsfile exists in the four folders checked..binfile run the same command as before.Thank you @philemonkipngetich for the very clear diagnosis in #11386. Your note that the SDK resolves but only
.exeand.bunxshims exist is exactly what this builds on, and your workaround of running the declaredbinentry withnodeis the same idea moved into the CLI.Scenarios Tested
src/deploy/functions/runtimes/node/index.spec.tsthat build real folders in a temp directory: an npm layout with.bin/firebase-functions, a Bun store with only.exeand.bunx, a pnpm layout where the root.binshim is absent (only.cmdand.ps1, since on Windows pnpm normally also writes the extensionless shim), abindeclared as a single path, an SDK with nobin, abinpointing to a missing file, and a malformedpackage.jsonboth above the entry point and at the SDK root. One more test runsspawnFunctionsProcess()against a bin file that prints its arguments and checks it receives exactly the source directory. Without the fix, the Bun, pnpm and single path cases fail withFailed to find location of Firebase Functions SDK.firebase-functions@7.4.0withbun install --linker isolated, replaced the extensionless shim with.exeand.bunxfiles to match the Windows layout, and ranDelegate.execAdmin()from the built CLI. It exited 0 and wrote afunctions.yamllisting thehellofunction. I do not have a Windows machine, so a run there would be very welcome.npm run build,npx tsc --noEmit, eslint and prettier on the changed files, andnpm run mocha:fast(one failure insrc/frameworks/utils.spec.tsthat also happens without this change in my environment, since it runsnpm root).Sample Commands
firebase deploy --only functionsCloses #11386