Separate adapter builds into helper function. #384

annajowang · 2025-08-15T16:27:57Z

No description provided.

gemini-code-assist

Summary of Changes

Hello @annajowang, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the build process for apphosting framework adapters. It extracts the core logic for fetching and executing framework-specific builds into a dedicated, reusable helper function. This helper is then integrated into the localbuild.ts script, which is updated to accept a framework argument, making the build process more modular and extensible. Additionally, a list of supported frameworks is introduced to ensure valid inputs.

Highlights

Introduction of adapterBuild Helper Function: A new file, adapter-builds.ts, has been added. It exports an asynchronous function adapterBuild that encapsulates the entire process of fetching the latest version of a given framework adapter from npm, logging its version, and then executing the framework-specific build command using npx.
Refactoring of localbuild.ts for Genericity: The localbuild.ts script has been significantly refactored. The previously inline and hardcoded adapter build logic for 'nextjs' has been replaced with a call to the new adapterBuild helper. The script now accepts a <framework> argument, making it more dynamic and reusable for different frameworks.
Definition of Supported Frameworks: A new constant array, SupportedFrameworks, has been added to packages/@apphosting/common/src/index.ts. This array currently lists 'nextjs' and 'angular' as supported frameworks and is used in localbuild.ts to validate the framework argument provided by the user.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request successfully refactors the adapter build logic into a reusable helper function, improving code structure. However, I've identified a critical issue where an asynchronous function call is not awaited, which would cause the program to terminate prematurely. There's also a high-severity issue with an unhandled error event in a child process. Additionally, I've provided suggestions to improve error messages, enhance type safety, and clean up unused code.

gemini-code-assist · 2025-08-15T16:29:27Z

packages/@apphosting/build/src/bin/localbuild.ts

+    if (SupportedFrameworks.includes(framework)) {
+      adapterBuild(framework, projectRoot)
+    }


There are two issues here:

The adapterBuild function is async but it's not being awaited. This is a critical bug as it will cause the script to exit before the build process completes.

If the provided framework is not in SupportedFrameworks, the script exits silently without any feedback to the user. It should print an error message and exit with a non-zero status code.

The suggested code addresses both points.

if (!SupportedFrameworks.includes(framework)) { console.error(`Framework "${framework}" is not supported. Supported frameworks are: ${SupportedFrameworks.join(", ")}`); process.exit(1); } await adapterBuild(framework, projectRoot);

gemini-code-assist · 2025-08-15T16:29:27Z

packages/@apphosting/build/src/adapter-builds.ts

+  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();
+    });
+  });


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(); }); });

gemini-code-assist · 2025-08-15T16:29:27Z

packages/@apphosting/build/src/adapter-builds.ts

@@ -0,0 +1,35 @@
+import { spawn } from "child_process";
+import { program } from "commander";


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

gemini-code-assist · 2025-08-15T16:29:28Z

packages/@apphosting/build/src/adapter-builds.ts

+  // 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}`);


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}`);

gemini-code-assist · 2025-08-15T16:29:28Z

packages/@apphosting/build/src/bin/localbuild.ts

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


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

gemini-code-assist · 2025-08-15T16:29:28Z

packages/@apphosting/common/src/index.ts

@@ -2,6 +2,9 @@
 import * as fs from "node:fs";
 import * as path from "node:path";

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


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;

- Add interfaces for the apphosting config, while sharing what can be shared with the bundle config - remove unused and duplicate interfaces in adapter-angular - Add 'Build' enum value for envvars availability. This was previously limited to "Runtime" because it was used only for bundle.yaml's envvars which are envvars set by the adapter after a build to be used for runtime. Now the envvars interface will be shared with the apphosting config

annajowang · 2025-08-15T21:01:17Z

packages/@apphosting/adapter-angular/src/interface.ts

@@ -9,27 +9,6 @@ export interface OutputBundleOptions {
  needsServerGenerated: boolean;
 }



github is the worst tool I have ever encountered.

Separate adapter builds into helper function.

60278c4

gemini-code-assist bot reviewed Aug 15, 2025

View reviewed changes

annajowang and others added 3 commits August 15, 2025 20:54

Separate adapter builds into helper function.

bb96354

parse yaml

a2608fc

annajowang changed the base branch from main to filter_node_modules August 15, 2025 20:59

annajowang changed the base branch from filter_node_modules to main August 15, 2025 20:59

annajowang commented Aug 15, 2025

View reviewed changes

annajowang changed the base branch from main to angular-adapter-fix August 15, 2025 21:02

annajowang changed the base branch from angular-adapter-fix to main August 15, 2025 21:03

Add e2e tests.

2290112

annajowang changed the base branch from main to filter_node_modules August 18, 2025 15:46

annajowang changed the base branch from filter_node_modules to main August 18, 2025 15:46

annajowang closed this Aug 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Separate adapter builds into helper function. #384

Separate adapter builds into helper function. #384

Uh oh!

annajowang commented Aug 15, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Aug 15, 2025

Uh oh!

gemini-code-assist bot Aug 15, 2025

Uh oh!

gemini-code-assist bot Aug 15, 2025

Uh oh!

gemini-code-assist bot Aug 15, 2025

Uh oh!

gemini-code-assist bot Aug 15, 2025

Uh oh!

gemini-code-assist bot Aug 15, 2025

Uh oh!

annajowang Aug 15, 2025

Uh oh!

Uh oh!

		@@ -0,0 +1,35 @@
		import { spawn } from "child_process";
		import { program } from "commander";

	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}`);

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

		@@ -9,27 +9,6 @@ export interface OutputBundleOptions {
		needsServerGenerated: boolean;
		}

Separate adapter builds into helper function. #384

Separate adapter builds into helper function. #384

Uh oh!

Conversation

annajowang commented Aug 15, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

annajowang Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!