Skip to content
Closed
26 changes: 16 additions & 10 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion packages/@apphosting/build/.gitignore

This file was deleted.

1 change: 1 addition & 0 deletions packages/@apphosting/build/e2e/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
runs
33 changes: 33 additions & 0 deletions packages/@apphosting/build/e2e/adapter-builds.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import * as assert from "assert";
import { posix } from "path";
import pkg from "@apphosting/common";
import { scenarios } from "./scenarios.ts";
import fsExtra from "fs-extra";
import { parse as parseYaml } from "yaml";

const { readFileSync } = fsExtra;
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { OutputBundleConfig } = pkg;
Comment on lines +3 to +10

Choose a reason for hiding this comment

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

high

This way of importing a type is confusing and can lead to runtime errors. Since OutputBundleConfig is only used as a type, you can use a type-only import, which is safer and clearer. This also allows you to remove the eslint-disable and the runtime destructuring.

Suggested change
import pkg from "@apphosting/common";
import { scenarios } from "./scenarios.ts";
import fsExtra from "fs-extra";
import { parse as parseYaml } from "yaml";
const { readFileSync } = fsExtra;
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { OutputBundleConfig } = pkg;
import type { OutputBundleConfig } from "@apphosting/common";
import { scenarios } from "./scenarios.ts";
import fsExtra from "fs-extra";
import { parse as parseYaml } from "yaml";
const { readFileSync } = fsExtra;


const scenario = process.env.SCENARIO;
if (!scenario) {
throw new Error("SCENARIO environment variable expected");
}

const runId = process.env.RUN_ID;
if (!runId) {
throw new Error("RUN_ID environment variable expected");
}

const bundleYaml = posix.join(process.cwd(), "e2e", "runs", runId, ".apphosting", "bundle.yaml");
describe("supported framework apps", () => {
it("apps have bundle.yaml correctly generated", () => {
const bundle: OutputBundleConfig = parseYaml(readFileSync(bundleYaml, "utf8"));

assert.deepStrictEqual(scenarios.get(scenario).expectedBundleYaml.runConfig, bundle.runConfig);
assert.deepStrictEqual(
scenarios.get(scenario).expectedBundleYaml.metadata.adapterPackageName,
bundle.metadata.adapterPackageName,
);
});
});
94 changes: 94 additions & 0 deletions packages/@apphosting/build/e2e/run-local-build.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import { cp } from "fs/promises";
import promiseSpawn from "@npmcli/promise-spawn";
import { dirname, join, relative } from "path";
import { fileURLToPath } from "url";
import fsExtra from "fs-extra";
import { scenarios } from "./scenarios.js";

const { readFileSync, mkdirp, rmdir } = fsExtra;

const __dirname = dirname(fileURLToPath(import.meta.url));

const errors: any[] = [];

Choose a reason for hiding this comment

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

medium

Using any is discouraged as it bypasses type checking. It's better to use a more specific type like Error or unknown to improve type safety.

const errors: Error[] = [];


await rmdir(join(__dirname, "runs"), { recursive: true }).catch(() => undefined);

// Run each scenario
for (const [scenarioName, scenario] of scenarios) {
console.log(
`\n\n${"=".repeat(80)}\n${" ".repeat(
5,
)}RUNNING SCENARIO: ${scenarioName.toUpperCase()}${" ".repeat(5)}\n${"=".repeat(80)}`,
);

const runId = `${scenarioName}-${Math.random().toString().split(".")[1]}`;
const cwd = join(__dirname, "runs", runId);
await mkdirp(cwd);

const starterTemplateDir = scenarioName.includes("nextjs")
? "../../../starters/nextjs/basic"
: "../../../starters/angular/basic";
console.log(`[${runId}] Copying ${starterTemplateDir} to working directory`);
await cp(starterTemplateDir, cwd, { recursive: true });

console.log(`[${runId}] > npm ci --silent --no-progress`);
await promiseSpawn("npm", ["ci", "--silent", "--no-progress"], {
cwd,
stdio: "inherit",
shell: true,
});

const buildScript = relative(cwd, join(__dirname, "../dist/bin/localbuild.js"));
const buildLogPath = join(cwd, "build.log");
console.log(`[${runId}] > node ${buildScript} (output written to ${buildLogPath})`);

const packageJson = JSON.parse(readFileSync(join(cwd, "package.json"), "utf-8"));
const frameworkVersion = scenarioName.includes("nextjs")
? packageJson.dependencies.next.replace("^", "")
: JSON.parse(
readFileSync(join(cwd, "node_modules", "@angular", "core", "package.json"), "utf-8"),
).version;

try {
const result = await promiseSpawn("node", [buildScript, ...scenario.inputs], {
cwd,
stdioString: true,
stdio: "pipe",
shell: true,
env: {
...process.env,
FRAMEWORK_VERSION: frameworkVersion,
},
});
// Write stdout and stderr to the log file
fsExtra.writeFileSync(buildLogPath, result.stdout + result.stderr);

try {
// Determine which test files to run
const testPattern = scenario.tests
? scenario.tests.map((test) => `e2e/${test}`).join(" ")
: "e2e/*.spec.ts";

console.log(`> SCENARIO=${scenarioName} ts-mocha -p tsconfig.json ${testPattern}`);

await promiseSpawn("ts-mocha", ["-p", "tsconfig.json", ...testPattern.split(" ")], {
shell: true,
stdio: "inherit",
env: {
...process.env,
SCENARIO: scenarioName,
RUN_ID: runId,
},
});
} catch (e) {
errors.push(e);
}
} catch (e) {
console.error(`Error in scenario ${scenarioName}:`, e);
errors.push(e);
}
}
if (errors.length) {
console.error(errors);

Choose a reason for hiding this comment

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

medium

Printing the errors array directly might not provide readable output for Error objects. Iterating over the array and logging each error individually would be more informative for debugging.

  errors.forEach((error) => console.error(error));

process.exit(1);
}
41 changes: 41 additions & 0 deletions packages/@apphosting/build/e2e/scenarios.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
interface Scenario {
inputs: string[];
expectedBundleYaml: {};
tests?: string[]; // List of test files to run
}
Comment on lines +1 to +5

Choose a reason for hiding this comment

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

medium

Using {} as a type is not very descriptive and is similar to any. It's better to use a more specific type. You could import OutputBundleConfig from @apphosting/common and use Partial<OutputBundleConfig> for expectedBundleYaml to improve type safety and code clarity.

import type { OutputBundleConfig } from "@apphosting/common";

interface Scenario {
  inputs: string[];
  expectedBundleYaml: Partial<OutputBundleConfig>;
  tests?: string[]; // List of test files to run
}


export const scenarios: Map<string, Scenario> = new Map([
[
"nextjs-app",
{
inputs: ["./", "--framework", "nextjs"],
expectedBundleYaml: {
version: "v1",
runConfig: {
runCommand: "node .next/standalone/server.js",
},
metadata: {
adapterPackageName: "@apphosting/adapter-nextjs",
},
},
tests: ["adapter-builds.spec.ts"],
},
],
[
"angular-app",
{
inputs: ["./", "--framework", "angular"],
expectedBundleYaml: {
version: "v1",
runConfig: {
runCommand: "node dist/firebase-app-hosting-angular/server/server.mjs",
environmentVariables: [],
},
metadata: {
adapterPackageName: "@apphosting/adapter-angular",
},
},
tests: ["adapter-builds.spec.ts"],
},
],
]);
17 changes: 13 additions & 4 deletions packages/@apphosting/build/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
"url": "git+https://github.com/FirebaseExtended/firebase-framework-tools.git"
},
"bin": {
"build": "dist/bin/build.js",
"apphosting-local-build": "dist/bin/localbuild.js"
"build": "dist/bin/build.js",
"apphosting-local-build": "dist/bin/localbuild.js"
},
"author": {
"name": "Firebase",
Expand All @@ -21,12 +21,18 @@
"type": "module",
"sideEffects": false,
"scripts": {
"build": "rm -rf dist && tsc && chmod +x ./dist/bin/*"
"build": "rm -rf dist && tsc && chmod +x ./dist/bin/*",
"test": "npm run test:functional",
"test:functional": "node --loader ts-node/esm ./e2e/run-local-build.ts"
},
"exports": {
".": {
"node": "./dist/index.js",
"default": null
},
"./dist/*": {
"node": "./dist/*",
"default": null
}
},
"files": [
Expand All @@ -41,6 +47,9 @@
"ts-node": "^10.9.1"
},
"devDependencies": {
"@types/commander": "*"
"@types/commander": "*",
"ts-mocha": "*",
"ts-node": "*",
"typescript": "*"
}
}
33 changes: 33 additions & 0 deletions packages/@apphosting/build/src/adapter-builds.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import promiseSpawn from "@npmcli/promise-spawn";
import { yellow, bgRed, bold } from "colorette";

export async function adapterBuild(projectRoot: string, framework: string) {

Choose a reason for hiding this comment

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

medium

For better type safety, consider using the Framework type from @apphosting/common for the framework parameter, instead of a generic string. This requires exporting the Framework type from @apphosting/common first, and adding import type { Framework } from '@apphosting/common'; to this file.

Suggested change
export async function adapterBuild(projectRoot: string, framework: string) {
export async function adapterBuild(projectRoot: string, framework: Framework) {

// TODO(#382): We are using the latest framework adapter versions, but in the future
// we should parse the framework version and use the matching adapter version.
const adapterName = `@apphosting/adapter-${framework}`;
const packumentResponse = await fetch(`https://registry.npmjs.org/${adapterName}`);
if (!packumentResponse.ok)
throw new Error(
`Failed to fetch ${adapterName}: ${packumentResponse.status} ${packumentResponse.statusText}`,
);
let packument;
try {
packument = await packumentResponse.json();
} catch (e) {
throw new Error(`Failed to parse response from NPM registry for ${adapterName}.`);
}
const adapterVersion = packument?.["dist-tags"]?.["latest"];
if (!adapterVersion) {
throw new Error(`Could not find 'latest' dist-tag for ${adapterName}`);
}
// TODO(#382): should check for existence of adapter in app's package.json and use that version instead.

console.log(" 🔥", bgRed(` ${adapterName}@${yellow(bold(adapterVersion))} `), "\n");

const buildCommand = `apphosting-adapter-${framework}-build`;
await promiseSpawn("npx", ["-y", "-p", `${adapterName}@${adapterVersion}`, buildCommand], {
cwd: projectRoot,
shell: true,
stdio: "inherit",
});
}
49 changes: 13 additions & 36 deletions packages/@apphosting/build/src/bin/localbuild.ts
Original file line number Diff line number Diff line change
@@ -1,46 +1,23 @@
#! /usr/bin/env node
import { spawn } from "child_process";
import { SupportedFrameworks } from "@apphosting/common";

Choose a reason for hiding this comment

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

medium

To improve type safety, import the Framework type as well. This will be used to cast the framework option before passing it to other functions.

Suggested change
import { SupportedFrameworks } from "@apphosting/common";
import { SupportedFrameworks, Framework } from "@apphosting/common";

import { adapterBuild } from "../adapter-builds.js";
import { program } from "commander";
import { yellow, bgRed, bold } from "colorette";

// TODO(#382): add framework option later or incorporate micro-discovery.
// TODO(#382): parse apphosting.yaml for environment variables / secrets.
// TODO(#382): parse apphosting.yaml for runConfig and include in buildSchema
// TODO(#382): Support custom build and run commands (parse and pass run command to build schema).
program
.argument("<projectRoot>", "path to the project's root directory")
.action(async (projectRoot: string) => {
const framework = "nextjs";
// TODO(#382): We are using the latest framework adapter versions, but in the future
// we should parse the framework version and use the matching adapter version.
const adapterName = `@apphosting/adapter-nextjs`;
const packumentResponse = await fetch(`https://registry.npmjs.org/${adapterName}`);
if (!packumentResponse.ok) throw new Error(`Something went wrong fetching ${adapterName}`);
const packument = await packumentResponse.json();
const adapterVersion = packument?.["dist-tags"]?.["latest"];
if (!adapterVersion) {
throw new Error(`Could not find 'latest' dist-tag for ${adapterName}`);
}
// TODO(#382): should check for existence of adapter in app's package.json and use that version instead.

console.log(" 🔥", bgRed(` ${adapterName}@${yellow(bold(adapterVersion))} `), "\n");
.option("--framework <framework>")
.action(async (projectRoot, opts) => {
// TODO(#382): support other apphosting.*.yaml files.

const buildCommand = `apphosting-adapter-${framework}-build`;
await new Promise<void>((resolve, reject) => {
const child = spawn("npx", ["-y", "-p", `${adapterName}@${adapterVersion}`, buildCommand], {
cwd: projectRoot,
shell: true,
stdio: "inherit",
});
// TODO(#382): parse apphosting.yaml for environment variables / secrets needed during build time.
if (opts.framework && SupportedFrameworks.includes(opts.framework)) {
// TODO(#382): Skip this if there's a custom build command in apphosting.yaml.
await adapterBuild(projectRoot, opts.framework);

Choose a reason for hiding this comment

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

medium

To ensure type safety when calling adapterBuild, cast opts.framework to the more specific Framework type. The includes check on line 13 makes this cast safe.

Suggested change
await adapterBuild(projectRoot, opts.framework);
await adapterBuild(projectRoot, opts.framework as Framework);

}

child.on("exit", (code) => {
if (code !== 0) {
reject(new Error(`framework adapter build failed with error code ${code}.`));
}
resolve();
});
});
// TODO(#382): parse bundle.yaml and apphosting.yaml and output a buildschema somewhere.
// TODO(#382): Parse apphosting.yaml to set custom run command in bundle.yaml
// TODO(#382): parse apphosting.yaml for environment variables / secrets needed during runtime to include in the bunde.yaml
// TODO(#382): parse apphosting.yaml for runConfig to include in bundle.yaml
});

program.parse();
9 changes: 7 additions & 2 deletions packages/@apphosting/build/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,18 @@
"noEmit": false,
"outDir": "dist",
"rootDir": "src"

},
"ts-node": {
"esm": true,
"logError": true,
"pretty": true
},
"include": [
"src/index.ts",
"src/bin/*.ts",
],
"exclude": [
"src/*.spec.ts"
"src/*.spec.ts",
"src/bin/build.ts"
]
}
2 changes: 1 addition & 1 deletion packages/@apphosting/common/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@apphosting/common",
"version": "0.0.7",
"version": "0.0.8",
"description": "Shared library code for App Hosting framework adapters",
"author": {
"name": "Firebase",
Expand Down
Loading
Loading