Skip to content

Conversation

@annajowang
Copy link
Collaborator

@annajowang annajowang commented Aug 12, 2025

apphosting-local-build for now just calls the nextjs adapter and gives clients something to call in early prototyping.

In the future, it should support all the apphosting magic in apphosting.yaml and output a buildschema that clients can plug into an API call.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @annajowang, I'm Gemini Code Assist1! 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 introduces a new command, apphosting-local-build, designed to streamline the local build process for projects. This command acts as a wrapper, automatically fetching and executing the appropriate framework adapter (currently Next.js) to perform the build within a specified project directory. This simplifies the developer workflow by abstracting away the need to manually manage framework adapter installations for local development and testing.

Highlights

  • New Command Introduction: A new command, apphosting-local-build, has been introduced, enabling users to trigger local builds for their projects using framework-specific adapters.
  • Dynamic Adapter Loading: The new command dynamically fetches and utilizes the latest canary version of the @apphosting/adapter-nextjs package, ensuring that local builds leverage the most up-to-date adapter logic.
  • Framework Specificity: The implementation currently hardcodes Next.js as the supported framework, with future plans to extend support for other frameworks.
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.

Footnotes

  1. 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.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new apphosting-local-build command to facilitate local builds. The implementation in packages/@apphosting/build/src/bin/localbuild.ts is a good start, but I've identified a few areas for improvement. My feedback focuses on removing unused code, improving argument handling, increasing robustness by adding checks for external data, and critically, ensuring the build process correctly waits for completion and propagates its exit status. These changes will make the new command more reliable and maintainable.

Comment on lines 30 to 34
spawn("npx", ["-y", "-p", `${adapterName}@${adapterVersion}`, buildCommand], {
cwd: projectRoot,
shell: true,
stdio: "inherit",
});

Choose a reason for hiding this comment

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

high

The spawn call is not awaited, which means the script will exit immediately without waiting for the npx build process to complete. This can lead to silent failures where the build fails but the script reports success. The child process should be awaited, and its exit code should be checked to determine if the build was successful.

    await new Promise<void>((resolve, reject) => {
      const child = spawn("npx", ["-y", "-p", `${adapterName}@${adapterVersion}`, buildCommand], {
        cwd: projectRoot,
        shell: true,
        stdio: "inherit",
      });
      child.on("close", (code) => {
        if (code === 0) {
          resolve();
        } else {
          reject(new Error(`Build process exited with code ${code}`));
        }
      });
      child.on("error", (err) => {
        reject(err);
      });
    });

Comment on lines 4 to 8
import { parse as semverParse } from "semver";
import { yellow, bgRed, bold } from "colorette";
// @ts-expect-error TODO add interface
import pickManifest from "npm-pick-manifest";
import { discover } from "@apphosting/discover";

Choose a reason for hiding this comment

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

medium

There are several unused imports (semverParse, pickManifest, discover). These should be removed to keep the code clean. The associated // @ts-expect-error comment for the unused pickManifest import can also be removed.

Suggested change
import { parse as semverParse } from "semver";
import { yellow, bgRed, bold } from "colorette";
// @ts-expect-error TODO add interface
import pickManifest from "npm-pick-manifest";
import { discover } from "@apphosting/discover";
import { yellow, bgRed, bold } from "colorette";

Comment on lines 13 to 15
.action(async () => {

const projectRoot = program.args[0];

Choose a reason for hiding this comment

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

medium

It's a better practice with commander to receive arguments as parameters in the .action() callback rather than accessing program.args. This improves readability and makes the handler less dependent on the program object's state.

Suggested change
.action(async () => {
const projectRoot = program.args[0];
.action(async (directory: string) => {
const projectRoot = directory;

Comment on lines 22 to 23
const packument = await packumentResponse.json();
const adapterVersion = packument["dist-tags"]["canary"];

Choose a reason for hiding this comment

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

medium

Accessing packument["dist-tags"]["canary"] directly can lead to a runtime error if the dist-tags property or the canary tag doesn't exist in the packument response. It's safer to add checks to ensure the properties exist before accessing them.

Suggested change
const packument = await packumentResponse.json();
const adapterVersion = packument["dist-tags"]["canary"];
const packument = await packumentResponse.json();
const adapterVersion = packument?.["dist-tags"]?.["canary"];
if (!adapterVersion) {
throw new Error(`Could not find 'canary' dist-tag for ${adapterName}.`);
}

@annajowang annajowang marked this pull request as ready for review August 13, 2025 21:18
@annajowang annajowang linked an issue Aug 13, 2025 that may be closed by this pull request
@@ -1,14 +1,15 @@
{
"name": "@apphosting/build",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want to reuse this package for what you want to do here it would probably be a good idea to clean it up a bit too. This was created a long time ago as a part of some prototype James was working on where we could call the adapter from Firebase Hosting classic but I doubt we're gonna go that route now.

I don't think the other code in src/bin is used anywhere afaik (might be good to double check) and we may want to delete it if this is a real package we're relying on now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1 planning to delete the old script.
james confirmed it's unused.

Copy link
Collaborator

@Yuangwang Yuangwang left a comment

Choose a reason for hiding this comment

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

stamping to unblock some testing but would be good to clean up this package a bit if we're gonna rely on it

@annajowang annajowang merged commit 62ec239 into main Aug 14, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement an apphosting-local-build command

2 participants