-
Notifications
You must be signed in to change notification settings - Fork 283
Create apphosting-local-build command. #383
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.
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
Conversation
There was a problem hiding this 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-nextjspackage, 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
-
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. ↩
There was a problem hiding this 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.
| spawn("npx", ["-y", "-p", `${adapterName}@${adapterVersion}`, buildCommand], { | ||
| cwd: projectRoot, | ||
| shell: true, | ||
| stdio: "inherit", | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
});
});| 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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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"; |
| .action(async () => { | ||
|
|
||
| const projectRoot = program.args[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| .action(async () => { | |
| const projectRoot = program.args[0]; | |
| .action(async (directory: string) => { | |
| const projectRoot = directory; |
| const packument = await packumentResponse.json(); | ||
| const adapterVersion = packument["dist-tags"]["canary"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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}.`); | |
| } |
| @@ -1,14 +1,15 @@ | |||
| { | |||
| "name": "@apphosting/build", | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Yuangwang
left a comment
There was a problem hiding this 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
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.