Skip to content

Commit

Permalink
Fix broken pnpm support for cf3 (#5467)
Browse files Browse the repository at this point in the history
Due to the way pnpm resolves packages with peer dependencies, location of the firebase functions sdk package isn't where the CLI expects it to be.

We make the logic for finding the binary associated Firebase Functions SDK more robust by checking list of possible paths where it might exist. We also add integration test for pnpm in the functions discovery test.

Fixes #5448
  • Loading branch information
taeold authored Jan 30, 2023
1 parent 5811aea commit accea7a
Show file tree
Hide file tree
Showing 8 changed files with 84 additions and 26 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
- Fixes issue where `init firestore` was unecessarilly checking for default resource location. (#5230 and #5452)
- Pass `trailingSlash` from Next.js config to `firebase.json` (#5445)
- Don't use Next.js internal redirects for the backend test (#5445)
- Fix issue where pnpm support broke for function emulation and deployment. (#5467)
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
10 changes: 10 additions & 0 deletions scripts/functions-discover-tests/fixtures/pnpm/functions/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
const functions = require("firebase-functions");
const { onRequest } = require("firebase-functions/v2/https");

exports.hellov1 = functions.https.onRequest((request, response) => {
response.send("Hello from Firebase!");
});

exports.hellov2 = onRequest((request, response) => {
response.send("Hello from Firebase!");
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "pnpm",
"dependencies": {
"firebase-functions": "^4.0.0"
},
"engines": {
"node": "16"
}
}
5 changes: 5 additions & 0 deletions scripts/functions-discover-tests/fixtures/pnpm/install.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#!/bin/bash
set -euxo pipefail # bash strict mode
IFS=$'\n\t'

cd functions && pnpm install
3 changes: 3 additions & 0 deletions scripts/functions-discover-tests/run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ firebase experiments:enable internaltesting
# Install yarn
npm i -g yarn

# Install pnpm
npm install -g pnpm --force # it's okay to reinstall pnpm

for dir in ./scripts/functions-discover-tests/fixtures/*; do
(cd $dir && ./install.sh)
done
Expand Down
10 changes: 10 additions & 0 deletions scripts/functions-discover-tests/tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,16 @@ describe("Function discovery test", function (this) {
},
],
},
{
name: "pnpm",
projectDir: "pnpm",
expects: [
{
codebase: "default",
endpoints: ["hellov1", "hellov2"],
},
],
},
];

for (const tc of testCases) {
Expand Down
71 changes: 45 additions & 26 deletions src/deploy/functions/runtimes/node/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import * as runtimes from "..";
import * as validate from "./validate";
import * as versioning from "./versioning";
import * as parseTriggers from "./parseTriggers";
import { fileExistsSync } from "../../../../fsutils";

const MIN_FUNCTIONS_SDK_VERSION = "3.20.0";

Expand Down Expand Up @@ -169,33 +170,51 @@ export class Delegate {
if (Object.keys(config || {}).length) {
env.CLOUD_RUNTIME_CONFIG = JSON.stringify(config);
}
// At this point, we've already confirmed that we found supported firebase functions sdk.
// Location of the binary included in the Firebase Functions SDK
// differs depending on the developer's setup and choice of package manager.
//
// We'll try few routes in the following order:
//
// 1. $SOURCE_DIR/node_modules/.bin/firebase-functions
// 2. node_modules closest to the resolved path ${require.resolve("firebase-functions")}
//
// (1) works for most package managers (npm, yarn[no-hoist],pnpm).
// (2) handles cases where developer prefers monorepo setup or bundled function code.
const sourceNodeModulesPath = path.join(this.sourceDir, "node_modules");
const sdkPath = require.resolve("firebase-functions", { paths: [this.sourceDir] });
// Find location of the closest node_modules/ directory where we found the sdk.
const binPath = sdkPath.substring(0, sdkPath.lastIndexOf("node_modules") + 12);
// And execute the binary included in the sdk.
const childProcess = spawn(path.join(binPath, ".bin", "firebase-functions"), [this.sourceDir], {
env,
cwd: this.sourceDir,
stdio: [/* stdin=*/ "ignore", /* stdout=*/ "pipe", /* stderr=*/ "inherit"],
});
childProcess.stdout?.on("data", (chunk) => {
logger.debug(chunk.toString());
});
return Promise.resolve(async () => {
const p = new Promise<void>((resolve, reject) => {
childProcess.once("exit", resolve);
childProcess.once("error", reject);
});

await fetch(`http://localhost:${port}/__/quitquitquit`);
setTimeout(() => {
if (!childProcess.killed) {
childProcess.kill("SIGKILL");
}
}, 10_000);
return p;
});
const sdkNodeModulesPath = sdkPath.substring(0, sdkPath.lastIndexOf("node_modules") + 12);
for (const nodeModulesPath of [sourceNodeModulesPath, sdkNodeModulesPath]) {
const binPath = path.join(nodeModulesPath, ".bin", "firebase-functions");
if (fileExistsSync(binPath)) {
logger.debug(`Found firebase-functions binary at '${binPath}'`);
const childProcess = spawn(binPath, [this.sourceDir], {
env,
cwd: this.sourceDir,
stdio: [/* stdin=*/ "ignore", /* stdout=*/ "pipe", /* stderr=*/ "inherit"],
});
childProcess.stdout?.on("data", (chunk) => {
logger.debug(chunk.toString());
});
return Promise.resolve(async () => {
const p = new Promise<void>((resolve, reject) => {
childProcess.once("exit", resolve);
childProcess.once("error", reject);
});

await fetch(`http://localhost:${port}/__/quitquitquit`);
setTimeout(() => {
if (!childProcess.killed) {
childProcess.kill("SIGKILL");
}
}, 10_000);
return p;
});
}
}
throw new FirebaseError(
"Failed to find location of Firebase Functions SDK. " +
"Please file a bug on Github (https://github.com/firebase/firebase-tools/)."
);
}

// eslint-disable-next-line require-await
Expand Down

0 comments on commit accea7a

Please sign in to comment.