Skip to content
Open
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
11 changes: 2 additions & 9 deletions firebase-vscode/webpack.common.js
Original file line number Diff line number Diff line change
Expand Up @@ -196,15 +196,8 @@ const extensionConfig = {
to: "./schema",
},
// TODO(hlshen): Sanity check if these should be fixed or removed. AFIACT, they exist for functions and hosting deploys, which are not relevant anymore.
// Copy uncompiled JS files called at runtime by
// firebase-tools/src/parseTriggers.ts
// {
// from: "*.js",
// to: "./",
// context: "../src/deploy/functions/runtimes/node",
// },
// // Copy cross-env-shell.js used to run predeploy scripts
// // to ensure they work in Windows
// Copy cross-env-shell.js used to run predeploy scripts
// to ensure they work in Windows
// {
// from: "../node_modules/cross-env/dist",
// to: "./cross-env/dist",
Expand Down
35 changes: 0 additions & 35 deletions src/deploy/functions/runtimes/node/extractTriggers.js

This file was deleted.

78 changes: 0 additions & 78 deletions src/deploy/functions/runtimes/node/extractTriggers.spec.js

This file was deleted.

59 changes: 59 additions & 0 deletions src/deploy/functions/runtimes/node/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as versioning from "./versioning";
import * as utils from "../../../../utils";
import { FirebaseError } from "../../../../error";
import { Runtime } from "../supported";
import * as discovery from "../discovery";

const PROJECT_ID = "test-project";
const PROJECT_DIR = "/some/path";
Expand Down Expand Up @@ -77,4 +78,62 @@ describe("NodeDelegate", () => {
expect(() => delegate.getNodeBinary()).to.throw(FirebaseError);
});
});

describe("discoverBuild", () => {
let getFunctionsSDKVersionMock: sinon.SinonStub;
let detectFromYamlMock: sinon.SinonStub;

beforeEach(() => {
getFunctionsSDKVersionMock = sinon.stub(versioning, "getFunctionsSDKVersion");
detectFromYamlMock = sinon.stub(discovery, "detectFromYaml");
});

afterEach(() => {
getFunctionsSDKVersionMock.restore();
detectFromYamlMock.restore();
});

it("throws error if SDK version is too old", async () => {
getFunctionsSDKVersionMock.returns("3.19.0");
const delegate = new node.Delegate(
PROJECT_ID,
PROJECT_DIR,
SOURCE_DIR,
"nodejs16" as Runtime,
);
try {
await delegate.discoverBuild({}, {});
throw new Error("Should have thrown");
} catch (err: any) {
expect(err).to.be.instanceOf(FirebaseError);
expect(err.message).to.include("Please update firebase-functions SDK");
}
});

it("proceeds if SDK version is valid", async () => {
getFunctionsSDKVersionMock.returns("4.0.0");
detectFromYamlMock.resolves({ endpoints: {} });
const delegate = new node.Delegate(
PROJECT_ID,
PROJECT_DIR,
SOURCE_DIR,
"nodejs16" as Runtime,
);
await delegate.discoverBuild({}, {});
expect(detectFromYamlMock).to.have.been.called;
});

it("proceeds if SDK version is invalid", async () => {
getFunctionsSDKVersionMock.returns("not-a-version");
detectFromYamlMock.resolves({ endpoints: {} });
const delegate = new node.Delegate(
PROJECT_ID,
PROJECT_DIR,
SOURCE_DIR,
"nodejs16" as Runtime,
);
await delegate.discoverBuild({}, {});
expect(detectFromYamlMock).to.have.been.called;
});
});
});
40 changes: 18 additions & 22 deletions src/deploy/functions/runtimes/node/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { DelegateContext } from "..";
import * as supported from "../supported";
import * as validate from "./validate";
import * as versioning from "./versioning";
import * as parseTriggers from "./parseTriggers";

import { fileExistsSync } from "../../../../fsutils";

// The versions of the Firebase Functions SDK that added support for the container contract.
Expand Down Expand Up @@ -292,27 +292,23 @@ export class Delegate {
env: backend.EnvironmentVariables,
): Promise<build.Build> {
if (!semver.valid(this.sdkVersion)) {
logger.debug(
`Could not parse firebase-functions version '${this.sdkVersion}' into semver. Falling back to parseTriggers.`,
);
return parseTriggers.discoverBuild(this.projectId, this.sourceDir, this.runtime, config, env);
}
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION)) {
logLabeledWarning(
"functions",
`You are using an old version of firebase-functions SDK (${this.sdkVersion}). ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION}`,
);
return parseTriggers.discoverBuild(this.projectId, this.sourceDir, this.runtime, config, env);
}
// Perform a check for the minimum SDK version that added annotation support for the `Build.extensions` property
// and log to the user explaining why they need to upgrade their version.
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES)) {
logLabeledBullet(
"functions",
`You are using a version of firebase-functions SDK (${this.sdkVersion}) that does not have support for the newest Firebase Extensions features. ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES} to use them correctly`,
);
logger.debug(`Could not parse firebase-functions version '${this.sdkVersion}' into semver.`);
} else {
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION)) {
throw new FirebaseError(
`You are using an old version of firebase-functions SDK (${this.sdkVersion}). ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION}`,
);
}
// Perform a check for the minimum SDK version that added annotation support for the `Build.extensions` property
// and log to the user explaining why they need to upgrade their version.
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES)) {
logLabeledBullet(
"functions",
`You are using a version of firebase-functions SDK (${this.sdkVersion}) that does not have support for the newest Firebase Extensions features. ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES} to use them correctly`,
);
}
}
Comment on lines 294 to 312
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

For better readability, you could invert the if/else to handle the valid semver case first. This avoids nesting the main logic within an else block and follows a more common pattern of handling the success path before the error/edge case path.

Suggested change
if (!semver.valid(this.sdkVersion)) {
logger.debug(
`Could not parse firebase-functions version '${this.sdkVersion}' into semver. Falling back to parseTriggers.`,
);
return parseTriggers.discoverBuild(this.projectId, this.sourceDir, this.runtime, config, env);
}
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION)) {
logLabeledWarning(
"functions",
`You are using an old version of firebase-functions SDK (${this.sdkVersion}). ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION}`,
);
return parseTriggers.discoverBuild(this.projectId, this.sourceDir, this.runtime, config, env);
}
// Perform a check for the minimum SDK version that added annotation support for the `Build.extensions` property
// and log to the user explaining why they need to upgrade their version.
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES)) {
logLabeledBullet(
"functions",
`You are using a version of firebase-functions SDK (${this.sdkVersion}) that does not have support for the newest Firebase Extensions features. ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES} to use them correctly`,
);
logger.debug(`Could not parse firebase-functions version '${this.sdkVersion}' into semver.`);
} else {
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION)) {
throw new FirebaseError(
`You are using an old version of firebase-functions SDK (${this.sdkVersion}). ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION}`,
);
}
// Perform a check for the minimum SDK version that added annotation support for the `Build.extensions` property
// and log to the user explaining why they need to upgrade their version.
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES)) {
logLabeledBullet(
"functions",
`You are using a version of firebase-functions SDK (${this.sdkVersion}) that does not have support for the newest Firebase Extensions features. ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES} to use them correctly`,
);
}
}
if (semver.valid(this.sdkVersion)) {
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION)) {
throw new FirebaseError(
`You are using an old version of firebase-functions SDK (${this.sdkVersion}). ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION}`,
);
}
// Perform a check for the minimum SDK version that added annotation support for the `Build.extensions` property
// and log to the user explaining why they need to upgrade their version.
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES)) {
logLabeledBullet(
"functions",
`You are using a version of firebase-functions SDK (${this.sdkVersion}) that does not have support for the newest Firebase Extensions features. ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES} to use them correctly`,
);
}
} else {
logger.debug(`Could not parse firebase-functions version '${this.sdkVersion}' into semver.`);
}

let discovered = await discovery.detectFromYaml(this.sourceDir, this.projectId, this.runtime);
if (!discovered) {
Expand Down
Loading
Loading