Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug where function discovery failed on projects using yarn workspaces. #5215

Merged
merged 1 commit into from
Dec 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"functions": {
"source": "packages/functions"
}
}
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'

yarn install
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"name": "yarn-workspace",
"private": true,
"workspaces": ["packages/functions", "packages/a"]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
exports.msg = "Hello world!";
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "a",
"version": "0.0.1"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
const functions = require("firebase-functions");
const { onRequest } = require("firebase-functions/v2/https");
const { msg } = require("a");

exports.hellov1 = functions.https.onRequest((request, response) => {
response.send(msg);
});

exports.hellov2 = onRequest((request, response) => {
response.send(msg);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"name": "simple",
"version": "0.0.1",
"dependencies": {
"firebase-functions": "4.0.0",
"firebase-admin": "^11.2.0",
"a": "latest"
},
"engines": {
"node": "16"
}
}
3 changes: 3 additions & 0 deletions scripts/functions-discover-tests/run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ IFS=$'\n\t'
# Unlock internal commands for discovering functions in a project.
firebase experiments:enable internaltesting

# Install yarn
npm i -g yarn

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 @@ -57,6 +57,16 @@ describe("Function discovery test", function (this) {
},
],
},
{
name: "yarn-workspaces",
projectDir: "yarn-workspaces",
expects: [
{
codebase: "default",
endpoints: ["hellov1", "hellov2"],
},
],
},
];

for (const tc of testCases) {
Expand Down
4 changes: 2 additions & 2 deletions src/deploy/functions/runtimes/node/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,9 @@ export class Delegate {
// Using a caching interface because we (may/will) eventually depend on the SDK version
// to decide whether to use the JS export method of discovery or the HTTP container contract
// method of discovery.
_sdkVersion = "";
_sdkVersion: string | undefined = undefined;
get sdkVersion() {
if (!this._sdkVersion) {
if (this._sdkVersion === undefined) {
this._sdkVersion = versioning.getFunctionsSDKVersion(this.sourceDir) || "";
}
return this._sdkVersion;
Expand Down
77 changes: 52 additions & 25 deletions src/deploy/functions/runtimes/node/versioning.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import * as _ from "lodash";
import * as fs from "fs";
import * as path from "path";

import * as clc from "colorette";
import * as semver from "semver";
import * as spawn from "cross-spawn";
Expand All @@ -7,17 +9,6 @@ import * as utils from "../../../../utils";
import { logger } from "../../../../logger";
import { track } from "../../../../track";

interface NpmListResult {
name: string;
dependencies: {
"firebase-functions": {
version: string;
from: string;
resolved: string;
};
};
}

interface NpmShowResult {
"dist-tags": {
latest: string;
Expand All @@ -34,30 +25,66 @@ export const FUNCTIONS_SDK_VERSION_TOO_OLD_WARNING =
clc.bold("npm i --save firebase-functions@latest") +
" in the functions folder.";

/**
* Exported for testing purposes only.
*
* @internal
*/
export function findModuleVersion(name: string, resolvedPath: string): string | undefined {
let searchPath = path.dirname(resolvedPath);
// eslint-disable-next-line no-constant-condition
while (true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think search path will always terminate with /, right? Why not use that as the condition instead of doing while (true)?

if (searchPath === "/" || path.basename(searchPath) === "node_modules") {
logger.debug(
`Failed to find version of module ${name}: reached end of search path ${searchPath}`
);
return;
}
const maybePackageJson = path.join(searchPath, "package.json");
if (fs.existsSync(maybePackageJson)) {
const pkg = require(maybePackageJson);
if (pkg.name === name) {
return pkg.version;
}
logger.debug(
`Failed to find version of module ${name}: instead found ${pkg.name} at ${searchPath}`
);
return;
}
searchPath = path.dirname(searchPath);
}
}

/**
* Returns the version of firebase-functions SDK specified by package.json and package-lock.json.
* @param sourceDir Source directory of functions code
* @return version string (e.g. "3.1.2"), or void if firebase-functions is not in package.json
* or if we had trouble getting the version.
*/
export function getFunctionsSDKVersion(sourceDir: string): string | void {
export function getFunctionsSDKVersion(sourceDir: string): string | undefined {
try {
const child = spawn.sync("npm", ["list", "firebase-functions", "--json=true"], {
cwd: sourceDir,
encoding: "utf8",
});
if (child.error) {
logger.debug("getFunctionsSDKVersion encountered error:", child.error.stack);
return;
}
const output: NpmListResult = JSON.parse(child.stdout);
return _.get(output, ["dependencies", "firebase-functions", "version"]);
return findModuleVersion(
"firebase-functions",
// Find the entry point of the firebase-function module. require.resolve works for project directories using
// npm, yarn (1), or yarn (1) workspaces. Does not support yarn (2) since GCF doesn't support it anyway:
// https://issuetracker.google.com/issues/213632942.
require.resolve("firebase-functions", { paths: [sourceDir] })
);
} catch (e: any) {
if (e.code === "MODULE_NOT_FOUND") {
utils.logLabeledWarning(
"functions",
"Couldn't find firebase-functions package in your source code. Have you run 'npm install'?"
);
}
logger.debug("getFunctionsSDKVersion encountered error:", e);
return;
}
}

/**
* Get latest version of the Firebase Functions SDK.
*/
export function getLatestSDKVersion(): string | undefined {
const child = spawn.sync("npm", ["show", "firebase-functions", "--json=true"], {
encoding: "utf8",
Expand All @@ -70,10 +97,10 @@ export function getLatestSDKVersion(): string | undefined {
return;
}
const output: NpmShowResult = JSON.parse(child.stdout);
if (_.isEmpty(output)) {
if (Object.keys(output).length === 0) {
return;
}
return _.get(output, ["dist-tags", "latest"]);
return output["dist-tags"]?.["latest"];
}

/**
Expand Down