From 74c1b19bfb8b6daedc4b9aacc6aeec1daa53134d Mon Sep 17 00:00:00 2001 From: Thomas Bouldin Date: Thu, 27 Oct 2022 14:29:46 -0700 Subject: [PATCH] Inlined.web frameworks label (#5176) * Move HostingResolved out of firebaseConfig * Move metric to deploy code and add label * Add some smoke tests to the hosting pepare library --- src/deploy/hosting/context.ts | 2 +- src/deploy/hosting/prepare.ts | 23 ++++- src/deploy/index.ts | 7 +- src/firebaseConfig.ts | 11 +-- src/frameworks/index.ts | 9 +- src/hosting/config.ts | 11 ++- src/hosting/options.ts | 3 +- src/test/deploy/hosting/prepare.spec.ts | 118 ++++++++++++++++++++++++ 8 files changed, 155 insertions(+), 29 deletions(-) create mode 100644 src/test/deploy/hosting/prepare.spec.ts diff --git a/src/deploy/hosting/context.ts b/src/deploy/hosting/context.ts index 05a69b07633..889df7b03b9 100644 --- a/src/deploy/hosting/context.ts +++ b/src/deploy/hosting/context.ts @@ -1,4 +1,4 @@ -import { HostingResolved } from "../../firebaseConfig"; +import { HostingResolved } from "../../hosting/config"; import { Context as FunctionsContext } from "../functions/args"; export interface HostingDeploy { diff --git a/src/deploy/hosting/prepare.ts b/src/deploy/hosting/prepare.ts index effbf065b9d..639ae6c84aa 100644 --- a/src/deploy/hosting/prepare.ts +++ b/src/deploy/hosting/prepare.ts @@ -6,6 +6,7 @@ import { Context } from "./context"; import { Options } from "../../options"; import { HostingOptions } from "../../hosting/options"; import { zipIn } from "../../functional"; +import { track } from "../../track"; /** * Prepare creates versions for each Hosting site to be deployed. @@ -25,12 +26,24 @@ export async function prepare(context: Context, options: HostingOptions & Option return Promise.resolve(); } - const version: Omit = { - status: "CREATED", - labels: deploymentTool.labels(), - }; const versions = await Promise.all( - configs.map((config) => api.createVersion(config.site, version)) + configs.map(async (config) => { + const labels: Record = { + ...deploymentTool.labels(), + }; + if (config.webFramework) { + labels["firebase-web-framework"] = config.webFramework; + } + const version: Omit = { + status: "CREATED", + labels, + }; + const [, versionName] = await Promise.all([ + track("hosting_deploy", config.webFramework || "classic"), + api.createVersion(config.site, version), + ]); + return versionName; + }) ); context.hosting = { deploys: [], diff --git a/src/deploy/index.ts b/src/deploy/index.ts index fc07e21d43d..e1e885f8e9d 100644 --- a/src/deploy/index.ts +++ b/src/deploy/index.ts @@ -61,11 +61,10 @@ export const deploy = async function ( if (targetNames.includes("hosting")) { const config = options.config.get("hosting"); - let deployedFrameworks: string[] = []; if (Array.isArray(config) ? config.some((it) => it.source) : config.source) { experiments.assertEnabled("webframeworks", "deploy a web framework to hosting"); const usedToTargetFunctions = targetNames.includes("functions"); - deployedFrameworks = await prepareFrameworks(targetNames, context, options); + await prepareFrameworks(targetNames, context, options); const nowTargetsFunctions = targetNames.includes("functions"); if (nowTargetsFunctions && !usedToTargetFunctions) { if (context.hostingChannel && !experiments.isEnabled("pintags")) { @@ -75,11 +74,7 @@ export const deploy = async function ( } await requirePermissions(TARGET_PERMISSIONS["functions"]); } - } else { - const count = Array.isArray(config) ? config.length : 1; - deployedFrameworks = Array(count).fill("classic"); } - await Promise.all(deployedFrameworks.map((framework) => track("hosting_deploy", framework))); } for (const targetName of targetNames) { diff --git a/src/firebaseConfig.ts b/src/firebaseConfig.ts index 58c9879d396..9a56e09a081 100644 --- a/src/firebaseConfig.ts +++ b/src/firebaseConfig.ts @@ -10,7 +10,7 @@ import { RequireAtLeastOne } from "./metaprogramming"; // should be sourced from - https://github.com/firebase/firebase-tools/blob/master/src/deploy/functions/runtimes/index.ts#L15 type CloudFunctionRuntimes = "nodejs10" | "nodejs12" | "nodejs14" | "nodejs16"; -type Deployable = { +export type Deployable = { predeploy?: string | string[]; postdeploy?: string | string[]; }; @@ -67,7 +67,7 @@ export type HostingHeaders = HostingSource & { }[]; }; -type HostingBase = { +export type HostingBase = { public?: string; source?: string; ignore?: string[]; @@ -101,13 +101,6 @@ export type HostingMultiple = (HostingBase & }> & Deployable)[]; -// After validating a HostingMultiple and resolving targets, we will instead -// have a HostingResolved. -export type HostingResolved = HostingBase & { - site: string; - target?: string; -} & Deployable; - type StorageSingle = { rules: string; target?: string; diff --git a/src/frameworks/index.ts b/src/frameworks/index.ts index 3b4bcd2ccdd..a3158d8e0db 100644 --- a/src/frameworks/index.ts +++ b/src/frameworks/index.ts @@ -248,7 +248,7 @@ export async function prepareFrameworks( context: any, options: any, emulators: EmulatorInfo[] = [] -): Promise { +): Promise { // `firebase-frameworks` requires Node >= 16. We must check for this to avoid horrible errors. const nodeVersion = process.version; if (!semver.satisfies(nodeVersion, ">=16.0.0")) { @@ -257,7 +257,6 @@ export async function prepareFrameworks( ); } - const deployedFrameworks: string[] = []; const project = needProjectId(context); const { projectRoot } = options; const account = getProjectDefaultAccount(projectRoot); @@ -284,12 +283,11 @@ export async function prepareFrameworks( const configs = hostingConfig(options); let firebaseDefaults: FirebaseDefaults | undefined = undefined; if (configs.length === 0) { - return deployedFrameworks; + return; } for (const config of configs) { const { source, site, public: publicDir } = config; if (!source) { - deployedFrameworks.push("classic"); continue; } config.rewrites ||= []; @@ -411,7 +409,7 @@ export async function prepareFrameworks( config.public = relative(projectRoot, hostingDist); if (wantsBackend) codegenFunctionsDirectory = codegenProdModeFunctionsDirectory; } - deployedFrameworks.push(`${framework}${codegenFunctionsDirectory ? "_ssr" : ""}`); + config.webFramework = `${framework}${codegenFunctionsDirectory ? "_ssr" : ""}`; if (codegenFunctionsDirectory) { if (firebaseDefaults) firebaseDefaults._authTokenSyncURL = "/__session"; @@ -555,7 +553,6 @@ exports.ssr = onRequest((req, res) => server.then(it => it.handle(req, res))); }); } } - return deployedFrameworks; } function codegenDevModeFunctionsDirectory() { diff --git a/src/hosting/config.ts b/src/hosting/config.ts index 6552cc04a08..c2707f4b0d8 100644 --- a/src/hosting/config.ts +++ b/src/hosting/config.ts @@ -5,7 +5,8 @@ import { FirebaseError } from "../error"; import { HostingMultiple, HostingSingle, - HostingResolved, + HostingBase, + Deployable, HostingRewrites, FunctionsRewrite, LegacyFunctionsRewrite, @@ -20,6 +21,14 @@ import * as path from "node:path"; import * as experiments from "../experiments"; import { logger } from "../logger"; +// After validating a HostingMultiple and resolving targets, we will instead +// have a HostingResolved. +export type HostingResolved = HostingBase & { + site: string; + target?: string; + webFramework?: string; +} & Deployable; + // assertMatches allows us to throw when an --only flag doesn't match a target // but an --except flag doesn't. Is this desirable behavior? function matchingConfigs( diff --git a/src/hosting/options.ts b/src/hosting/options.ts index 9cee6436858..2b54bf82f80 100644 --- a/src/hosting/options.ts +++ b/src/hosting/options.ts @@ -1,6 +1,7 @@ -import { FirebaseConfig, HostingResolved } from "../firebaseConfig"; +import { FirebaseConfig } from "../firebaseConfig"; import { Implements } from "../metaprogramming"; import { Options } from "../options"; +import { HostingResolved } from "./config"; /** * The set of fields that the Hosting codebase needs from Options. diff --git a/src/test/deploy/hosting/prepare.spec.ts b/src/test/deploy/hosting/prepare.spec.ts new file mode 100644 index 00000000000..8a72f814c77 --- /dev/null +++ b/src/test/deploy/hosting/prepare.spec.ts @@ -0,0 +1,118 @@ +import { expect } from "chai"; +import * as sinon from "sinon"; + +import { FirebaseConfig } from "../../../firebaseConfig"; +import { HostingOptions } from "../../../hosting/options"; +import { Context } from "../../../deploy/hosting/context"; +import { Options } from "../../../options"; +import * as hostingApi from "../../../hosting/api"; +import * as tracking from "../../../track"; +import * as deploymentTool from "../../../deploymentTool"; +import * as config from "../../../hosting/config"; +import { prepare } from "../../../deploy/hosting"; + +describe("hosting prepare", () => { + let hostingStub: sinon.SinonStubbedInstance; + let trackingStub: sinon.SinonStubbedInstance; + let siteConfig: config.HostingResolved; + let firebaseJson: FirebaseConfig; + let options: HostingOptions & Options; + + beforeEach(() => { + hostingStub = sinon.stub(hostingApi); + trackingStub = sinon.stub(tracking); + + // We're intentionally using pointer references so that editing site + // edits the results of hostingConfig() and changes firebase.json + siteConfig = { + site: "site", + public: ".", + }; + firebaseJson = { + hosting: siteConfig, + }; + options = { + cwd: ".", + configPath: ".", + only: "", + except: "", + filteredTargets: ["HOSTING"], + force: false, + json: false, + nonInteractive: false, + interactive: true, + debug: false, + config: { + src: firebaseJson, + } as any, + rc: null as any, + + // Forces caching behavior of hostingConfig call + normalizedHostingConfig: [siteConfig], + }; + }); + + afterEach(() => { + sinon.verifyAndRestore(); + }); + + it("passes a smoke test with web framework", async () => { + siteConfig.webFramework = "fake-framework"; + + // Edit the in-memory config to add a web framework + hostingStub.createVersion.callsFake((siteId, version) => { + expect(siteId).to.equal(siteConfig.site); + expect(version.status).to.equal("CREATED"); + expect(version.labels).to.deep.equal({ + ...deploymentTool.labels(), + "firebase-web-framework": "fake-framework", + }); + return Promise.resolve("version"); + }); + + const context: Context = { + projectId: "project", + }; + await prepare(context, options); + + expect(trackingStub.track).to.have.been.calledOnceWith("hosting_deploy", "fake-framework"); + expect(hostingStub.createVersion).to.have.been.calledOnce; + expect(context.hosting).to.deep.equal({ + deploys: [ + { + config: siteConfig, + version: "version", + }, + ], + }); + }); + + it("passes a smoke test without web framework", async () => { + // Do not set a web framework on siteConfig + + // Edit the in-memory config to add a web framework + hostingStub.createVersion.callsFake((siteId, version) => { + expect(siteId).to.equal(siteConfig.site); + expect(version.status).to.equal("CREATED"); + // Note: we're missing the web framework label + expect(version.labels).to.deep.equal(deploymentTool.labels()); + return Promise.resolve("version"); + }); + + const context: Context = { + projectId: "project", + }; + await prepare(context, options); + + expect(trackingStub.track).to.have.been.calledOnceWith("hosting_deploy", "classic"); + expect(hostingStub.createVersion).to.have.been.calledOnce; + expect(context.hosting).to.deep.equal({ + deploys: [ + { + config: siteConfig, + version: "version", + }, + ], + }); + }); +});