From a9c586b689c8ace3808928a695ad502b89012fd6 Mon Sep 17 00:00:00 2001 From: Philip Su <39933441+fivecar@users.noreply.github.com> Date: Tue, 4 Feb 2025 17:40:08 -0800 Subject: [PATCH 1/4] fix: #8168 - enforce webframeworks only when needed In deployments where `--only hosting:boo` is used, enforce webframeworks enablement only when the target actually uses webframeworks. --- src/deploy/index.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/deploy/index.ts b/src/deploy/index.ts index d422327b443..a63ea35c049 100644 --- a/src/deploy/index.ts +++ b/src/deploy/index.ts @@ -69,7 +69,17 @@ export const deploy = async function ( if (targetNames.includes("hosting")) { const config = options.config.get("hosting"); - if (Array.isArray(config) ? config.some((it) => it.source) : config.source) { + if ( + Array.isArray(config) + ? config.some( + (it) => + it.source && + (!options.only.includes("hosting:") || + new RegExp(`\\bhosting:${it.target}\\b`).exec(options.only)), + ) + : config.source + ) { + console.log("preparing webframeworks"); experiments.assertEnabled("webframeworks", "deploy a web framework from source"); await prepareFrameworks("deploy", targetNames, context, options); } From 7b68a0e9d37b2365c70471066765788180705aa0 Mon Sep 17 00:00:00 2001 From: Philip Su <39933441+fivecar@users.noreply.github.com> Date: Tue, 4 Feb 2025 17:49:31 -0800 Subject: [PATCH 2/4] remove console --- src/deploy/index.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/deploy/index.ts b/src/deploy/index.ts index a63ea35c049..12cb52c6e22 100644 --- a/src/deploy/index.ts +++ b/src/deploy/index.ts @@ -79,7 +79,6 @@ export const deploy = async function ( ) : config.source ) { - console.log("preparing webframeworks"); experiments.assertEnabled("webframeworks", "deploy a web framework from source"); await prepareFrameworks("deploy", targetNames, context, options); } From 6e8b10b16d9d54eed117ceedf61c8cd14295855c Mon Sep 17 00:00:00 2001 From: Philip Su <39933441+fivecar@users.noreply.github.com> Date: Wed, 5 Feb 2025 15:50:41 -0800 Subject: [PATCH 3/4] add changelog, add tests --- CHANGELOG.md | 1 + src/deploy/hosting/deploy.spec.ts | 104 ++++++++++++++++++++++++++++++ src/deploy/index.ts | 48 ++++++++------ 3 files changed, 134 insertions(+), 19 deletions(-) create mode 100644 src/deploy/hosting/deploy.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index e69de29bb2d..408ade04d69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -0,0 +1 @@ +- Enforce webframeworks enablement only on webframeworks sites (#8168) \ No newline at end of file diff --git a/src/deploy/hosting/deploy.spec.ts b/src/deploy/hosting/deploy.spec.ts new file mode 100644 index 00000000000..4bc17286bc2 --- /dev/null +++ b/src/deploy/hosting/deploy.spec.ts @@ -0,0 +1,104 @@ +import { expect } from "chai"; +import * as sinon from "sinon"; + +import { setEnabled } from "../../experiments"; +import { FirebaseConfig } from "../../firebaseConfig"; +import * as frameworks from "../../frameworks"; +import * as config from "../../hosting/config"; +import { HostingOptions } from "../../hosting/options"; +import { Options } from "../../options"; +import { prepareFrameworksIfNeeded } from "../index"; + +describe("hosting prepare", () => { + let frameworksStub: sinon.SinonStubbedInstance; + let classicSiteConfig: config.HostingResolved; + let webFrameworkSiteConfig: config.HostingResolved; + let firebaseJson: FirebaseConfig; + let options: HostingOptions & Options; + + beforeEach(() => { + frameworksStub = sinon.stub(frameworks); + + // We're intentionally using pointer references so that editing site + // edits the results of hostingConfig() and changes firebase.json + classicSiteConfig = { + site: "classic", + target: "classic", + public: ".", + }; + webFrameworkSiteConfig = { + site: "webframework", + target: "webframework", + source: "src", + }; + firebaseJson = { + hosting: [classicSiteConfig, webFrameworkSiteConfig], + }; + options = { + cwd: ".", + configPath: ".", + only: "hosting", + except: "", + filteredTargets: ["HOSTING"], + force: false, + json: false, + nonInteractive: false, + interactive: true, + debug: false, + projectId: "project", + config: { + src: firebaseJson, + get: (key: string) => { + if (key === "hosting") { + return firebaseJson.hosting; + } + return null; + }, + } as any, + rc: null as any, + + // Forces caching behavior of hostingConfig call + normalizedHostingConfig: [classicSiteConfig, webFrameworkSiteConfig], + }; + }); + + afterEach(() => { + sinon.verifyAndRestore(); + }); + + it("deploys classic site without webframeworks disabled", async () => { + setEnabled("webframeworks", false); + options.only = "hosting:classic"; + await expect(prepareFrameworksIfNeeded(["hosting"], options, {})).to.not.be.rejected; + }); + + it("fails webframework deploy with webframeworks disabled", async () => { + setEnabled("webframeworks", false); + options.only = "hosting:webframework"; + await expect(prepareFrameworksIfNeeded(["hosting"], options, {})).to.be.rejectedWith( + /Cannot deploy a web framework from source because the experiment.+webframeworks.+is not enabled/, + ); + }); + + it("deploys webframework site with webframeworks enabled", async () => { + setEnabled("webframeworks", true); + options.only = "hosting:webframework"; + await expect(prepareFrameworksIfNeeded(["hosting"], options, {})).to.not.be.rejected; + expect(frameworksStub.prepareFrameworks).to.have.been.calledOnceWith("deploy", ["hosting"]); + }); + + it("deploys classic site with webframeworks enabled", async () => { + setEnabled("webframeworks", true); + options.only = "hosting:classic"; + await expect(prepareFrameworksIfNeeded(["hosting"], options, {})).to.not.be.rejected; + expect(frameworksStub.prepareFrameworks).to.not.have.been.called; + }); + + it("fails when at least one site has webframeworks enabled and the experiment is disabled", async () => { + setEnabled("webframeworks", false); + options.only = "hosting"; + await expect(prepareFrameworksIfNeeded(["hosting"], options, {})).to.be.rejectedWith( + /Cannot deploy a web framework from source because the experiment.+webframeworks.+is not enabled/, + ); + }); +}); diff --git a/src/deploy/index.ts b/src/deploy/index.ts index 12cb52c6e22..34c7b935b77 100644 --- a/src/deploy/index.ts +++ b/src/deploy/index.ts @@ -17,13 +17,15 @@ import * as StorageTarget from "./storage"; import * as RemoteConfigTarget from "./remoteconfig"; import * as ExtensionsTarget from "./extensions"; import * as DataConnectTarget from "./dataconnect"; +import { HostingConfig } from "../firebaseConfig"; import { prepareFrameworks } from "../frameworks"; -import { HostingDeploy } from "./hosting/context"; +import { FrameworkContext } from "../frameworks/interfaces"; import { addPinnedFunctionsToOnlyString, hasPinnedFunctions } from "./hosting/prepare"; import { isRunningInGithubAction } from "../init/features/hosting/github"; import { TARGET_PERMISSIONS } from "../commands/deploy"; import { requirePermissions } from "../requirePermissions"; import { Options } from "../options"; +import { Context } from "./hosting/context"; const TARGETS = { hosting: HostingTarget, @@ -46,6 +48,27 @@ const chain = async function (fns: Chain, context: any, options: any, payload: a } }; +export const prepareFrameworksIfNeeded = async function ( + targetNames: (keyof typeof TARGETS)[], + options: DeployOptions, + context: FrameworkContext, +): Promise { + const config = options.config.get("hosting") as HostingConfig; + if ( + Array.isArray(config) + ? config.some( + (it) => + it.source && + (!options.only.includes("hosting:") || + new RegExp(`\\bhosting:${it.target ?? ""}\\b`).exec(options.only)), + ) + : config.source + ) { + experiments.assertEnabled("webframeworks", "deploy a web framework from source"); + await prepareFrameworks("deploy", targetNames, context, options); + } +}; + /** * The `deploy()` function runs through a three step deploy process for a listed * number of deploy targets. This allows deploys to be done all together or @@ -59,7 +82,7 @@ export const deploy = async function ( const projectId = needProjectId(options); const payload = {}; // a shared context object for deploy targets to decorate as needed - const context: any = Object.assign({ projectId }, customContext); + const context: Context = Object.assign({ projectId }, customContext); const predeploys: Chain = []; const prepares: Chain = []; const deploys: Chain = []; @@ -68,20 +91,7 @@ export const deploy = async function ( const startTime = Date.now(); if (targetNames.includes("hosting")) { - const config = options.config.get("hosting"); - if ( - Array.isArray(config) - ? config.some( - (it) => - it.source && - (!options.only.includes("hosting:") || - new RegExp(`\\bhosting:${it.target}\\b`).exec(options.only)), - ) - : config.source - ) { - experiments.assertEnabled("webframeworks", "deploy a web framework from source"); - await prepareFrameworks("deploy", targetNames, context, options); - } + await prepareFrameworksIfNeeded(targetNames, options, context); } if (targetNames.includes("hosting") && hasPinnedFunctions(options)) { @@ -157,11 +167,11 @@ export const deploy = async function ( const deployedHosting = includes(targetNames, "hosting"); logger.info(bold("Project Console:"), consoleUrl(options.project ?? "_", "/overview")); if (deployedHosting) { - each(context.hosting.deploys as HostingDeploy[], (deploy) => { + each(context.hosting?.deploys, (deploy) => { logger.info(bold("Hosting URL:"), addSubdomain(hostingOrigin(), deploy.config.site)); }); - const versionNames = context.hosting.deploys.map((deploy: any) => deploy.version); - return { hosting: versionNames.length === 1 ? versionNames[0] : versionNames }; + const versionNames = context.hosting?.deploys.map((deploy: any) => deploy.version); + return { hosting: versionNames?.length === 1 ? versionNames[0] : versionNames }; } else { return { hosting: undefined }; } From 00cc1fc2183bb62f31cb16928e438a4f6cb2d503 Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Fri, 14 Feb 2025 12:47:22 +0100 Subject: [PATCH 4/4] Add matchesHostingTarget to improve readability --- src/deploy/hosting/deploy.spec.ts | 20 +++++++++++++++++++- src/deploy/index.ts | 14 ++++++++------ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/src/deploy/hosting/deploy.spec.ts b/src/deploy/hosting/deploy.spec.ts index 4bc17286bc2..7770af2e96c 100644 --- a/src/deploy/hosting/deploy.spec.ts +++ b/src/deploy/hosting/deploy.spec.ts @@ -7,7 +7,7 @@ import * as frameworks from "../../frameworks"; import * as config from "../../hosting/config"; import { HostingOptions } from "../../hosting/options"; import { Options } from "../../options"; -import { prepareFrameworksIfNeeded } from "../index"; +import { matchesHostingTarget, prepareFrameworksIfNeeded } from "../index"; describe("hosting prepare", () => { let frameworksStub: sinon.SinonStubbedInstance; @@ -66,6 +66,24 @@ describe("hosting prepare", () => { sinon.verifyAndRestore(); }); + describe("matchesHostingTarget", () => { + it("should correctly identify if a hosting target should be included in deployment", () => { + const cases = [ + { only: "hosting:site1", target: "site1", expected: true }, + { only: "hosting:site12", target: "site1", expected: false }, + { only: "hosting:", target: "", expected: true }, + { only: "functions:fn1", target: "site1", expected: true }, + { only: "hosting:site1,hosting:site2", target: "site1", expected: true }, + { only: undefined, target: "site1", expected: true }, + { only: "hosting:site1,functions:fn1", target: "site1", expected: true }, + ]; + + cases.forEach(({ only, target, expected }) => { + expect(matchesHostingTarget(only, target)).to.equal(expected); + }); + }); + }); + it("deploys classic site without webframeworks disabled", async () => { setEnabled("webframeworks", false); options.only = "hosting:classic"; diff --git a/src/deploy/index.ts b/src/deploy/index.ts index 34c7b935b77..ab2eb6681b7 100644 --- a/src/deploy/index.ts +++ b/src/deploy/index.ts @@ -48,6 +48,13 @@ const chain = async function (fns: Chain, context: any, options: any, payload: a } }; +export const matchesHostingTarget = (only: string | undefined, target?: string): boolean => { + if (!only) return true; + if (!only.includes("hosting:")) return true; + const targetStr = `hosting:${target ?? ""}`; + return only.split(",").some((t) => t === targetStr); +}; + export const prepareFrameworksIfNeeded = async function ( targetNames: (keyof typeof TARGETS)[], options: DeployOptions, @@ -56,12 +63,7 @@ export const prepareFrameworksIfNeeded = async function ( const config = options.config.get("hosting") as HostingConfig; if ( Array.isArray(config) - ? config.some( - (it) => - it.source && - (!options.only.includes("hosting:") || - new RegExp(`\\bhosting:${it.target ?? ""}\\b`).exec(options.only)), - ) + ? config.some((it) => it.source && matchesHostingTarget(options.only, it.target)) : config.source ) { experiments.assertEnabled("webframeworks", "deploy a web framework from source");