Skip to content

Separate adapter builds into helper function. #384

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Separate adapter builds into helper function. #384

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
60278c4
Separate adapter builds into helper function.
annajowang Aug 15, 2025
853cc56
Add apphosting config interface to common. (#385)
annajowang Aug 15, 2025
bb96354
Separate adapter builds into helper function.
annajowang Aug 15, 2025
a2608fc
parse yaml
annajowang Aug 15, 2025
2290112
Add e2e tests.
annajowang Aug 17, 2025
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
21 changes: 0 additions & 21 deletions packages/@apphosting/adapter-angular/src/interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,27 +9,6 @@ export interface OutputBundleOptions {
needsServerGenerated: boolean;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

github is the worst tool I have ever encountered.

// Environment variable schema for bundle.yaml outputted by angular adapter
export interface EnvironmentVariable {
variable: string;
value: string;
availability: Availability.Runtime; // currently support RUNTIME only
}

// defines whether the environment variable is buildtime, runtime or both
export enum Availability {
Buildtime = "BUILD",
Runtime = "RUNTIME",
}

// Metadata schema for bundle.yaml outputted by angular adapter
export interface Metadata {
adapterPackageName: string;
adapterVersion: string;
framework: string;
frameworkVersion: string;
}

// valid manifest schema
export interface ValidManifest {
errors: string[];
Expand Down
35 changes: 35 additions & 0 deletions packages/@apphosting/build/src/adapter-builds.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { spawn } from "child_process";
import { program } from "commander";
Copy link

Choose a reason for hiding this comment

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

medium

The program import from commander is not used in this file and should be removed to keep the code clean.

import { yellow, bgRed, bold } from "colorette";

export async function adapterBuild(projectRoot: string, framework: string) {
// 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(`Something went wrong fetching ${adapterName}`);
Copy link

Choose a reason for hiding this comment

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

medium

The error message here is quite generic. It would be more helpful for debugging to include the HTTP status code and text in the error message.

Suggested change
if (!packumentResponse.ok) throw new Error(`Something went wrong fetching ${adapterName}`);
if (!packumentResponse.ok) throw new Error(`Failed to fetch ${adapterName}: ${packumentResponse.status} ${packumentResponse.statusText}`);

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");

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",
});

child.on("exit", (code) => {
if (code !== 0) {
reject(new Error(`framework adapter build failed with error code ${code}.`));
}
resolve();
});
});
Copy link

Choose a reason for hiding this comment

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

high

The spawn function can emit an 'error' event if it fails to start the process (e.g., if npx is not found). This event is not currently handled, which could lead to an unhandled promise rejection. You should add an error handler for the child process to ensure all failure modes are caught.

  await new Promise<void>((resolve, reject) => {
    const child = spawn("npx", ["-y", "-p", `${adapterName}@${adapterVersion}`, buildCommand], {
      cwd: projectRoot,
      shell: true,
      stdio: "inherit",
    });

    child.on("error", reject);

    child.on("exit", (code) => {
      if (code !== 0) {
        reject(new Error(`framework adapter build failed with error code ${code}.`));
      }
      resolve();
    });
  });

}
54 changes: 20 additions & 34 deletions packages/@apphosting/build/src/bin/localbuild.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,45 +2,31 @@
import { spawn } from "child_process";
import { program } from "commander";
import { yellow, bgRed, bold } from "colorette";
Copy link

Choose a reason for hiding this comment

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

medium

The imports for spawn, yellow, bgRed, and bold are no longer used in this file after the refactoring. They should be removed to maintain code cleanliness.

Suggested change
import { spawn } from "child_process";
import { program } from "commander";
import { yellow, bgRed, bold } from "colorette";
import { program } from "commander";

import { SupportedFrameworks, ApphostingConfig } from "@apphosting/common";
import { adapterBuild } from "../adapter-builds.js";
import { parse as parseYaml } from "yaml";
import fsExtra from "fs-extra";
import { join } from "path";

export const { readFileSync } = fsExtra;

// 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.
.argument("<projectRoot>", "path to the project's root directory")
.option("<framework>", "the framework to build for")
.action(async (projectRoot: string, opts: any) => {
// TODO(#382): support other apphosting.*.yaml files.
const apphostingYaml = parseYaml(readFileSync(join(projectRoot, "apphosting.yaml")).toString()) as ApphostingConfig;

console.log(" 🔥", bgRed(` ${adapterName}@${yellow(bold(adapterVersion))} `), "\n");
// TODO(#382): parse apphosting.yaml for environment variables / secrets needed during build time.

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",
});
if (opts.framwork && SupportedFrameworks.includes(opts.framework)) {
// TODO(#382): Skip this if there's a custom build command in apphosting.yaml.
adapterBuild(projectRoot, opts.framework)
}

child.on("exit", (code) => {
if (code !== 0) {
reject(new Error(`framework adapter build failed with error code ${code}.`));
}
resolve();
});
// 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
});
// TODO(#382): parse bundle.yaml and apphosting.yaml and output a buildschema somewhere.
});

program.parse();
34 changes: 31 additions & 3 deletions packages/@apphosting/common/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@ import { spawn } from "child_process";
import * as fs from "node:fs";
import * as path from "node:path";

// List of apphosting supported frameworks.
export const SupportedFrameworks = ['nextjs', 'angular'];
Copy link

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, you can use as const to treat SupportedFrameworks as a readonly tuple of string literals. This allows TypeScript to perform more specific type checking when this array is used.

Suggested change
export const SupportedFrameworks = ['nextjs', 'angular'];
export const SupportedFrameworks = ['nextjs', 'angular'] as const;


// **** OutputBundleConfig interfaces ****

// Output bundle metadata specifications to be written to bundle.yaml
export interface OutputBundleConfig {
version: "v1";
Expand Down Expand Up @@ -41,9 +46,31 @@ export interface Metadata {
frameworkVersion?: string;
}

// **** Apphosting Config interfaces ****

export interface ApphostingConfig {
runconfig?: ApphostingRunConfig;
env?: EnvVarConfig[];
scripts?: Script;
outputFiles?: OutputFiles;
}

export interface ApphostingRunConfig {
minInstances?: number;
maxInstances?: number;
concurrency?: number;
}

export interface Script {
buildCommand?: string;
runCommand?: string;
}

// **** Shared interfaces ****

// Optional outputFiles to configure outputFiles and optimize server files + static assets.
// If this is not set then all of the source code will be uploaded
interface OutputFiles {
export interface OutputFiles {
serverApp: ServerApp;
}

Expand All @@ -59,14 +86,15 @@ export interface EnvVarConfig {
variable: string;
// Value associated with the variable
value: string;
// Where the variable will be available, for now only RUNTIME is supported
availability: Availability.Runtime[];
// Where the variable will be available
availability: Availability[];
}

// Represents where environment variables are made available
export enum Availability {
// Runtime environment variables are available on the server when the app is run
Runtime = "RUNTIME",
Build = "BUILD",
}

// Options to configure the build of a framework application
Expand Down