Conversation
…to the local build process
1. Fix for injecting auto-init variables into the build 2. Fixes how we handle dependencies, nodejs paths, modulepaths, etc. This needs closer attention/fixes. 3. Adds env var handling (not secrets) and determines which env vars to pass down to the build
… variants) and include them in the final artifact
…zip_deploy_aryanf
Summary of ChangesHello, 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 significantly enhances the local development and deployment experience for Firebase App Hosting applications. It refines the local build process by enabling dynamic environment variable injection and proper Node.js module resolution, making local testing more robust. Furthermore, the deployment mechanism has been upgraded to use Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements for Firebase App Hosting, primarily enabling local builds and deployment of the resulting artifacts. Key changes include updating the packaging format to .tar.gz, integrating the local build process, and improving environment variable handling from apphosting.yaml files for both build and runtime. The changes also include refactoring configuration loading for better reusability and enforcing stricter security on GCS buckets. My review found one medium-severity issue related to a style guide violation where as any is used as a type-casting escape hatch.
src/gcp/storage.ts
Outdated
| !managedBucket.iamConfiguration?.uniformBucketLevelAccess?.enabled | ||
| ) { | ||
| await dynamicDispatch.patchBucket(managedBucket.name, { | ||
| iamConfiguration: opts.req.iamConfiguration as any, |
There was a problem hiding this comment.
The use of as any here is an escape hatch that violates the repository's style guide, which states: "Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards." (GEMINI.md:38).
The type of opts.req.iamConfiguration from UpsertBucketRequest is not fully compatible with BucketResponse['iamConfiguration'] which is expected by patchBucket. To resolve this and improve type safety, consider defining a more specific type for the patch operation, or adjusting the existing types to be compatible.
For example, you could define a patch-specific type for iamConfiguration.
A similar issue exists on line 491.
References
- The style guide prohibits using
anyorunknownas a type escape hatch. Proper interfaces or types should be defined instead. (link)
…ctually a local build (instead of assuming true.)
…ore strictly. We also remove some hardcoded values and we generalize the code so that it does not affect source deploys. The goal is to prepare this PR so that we can safely submit it to main (behind the experiment flag.)
…ive. Only run it if it's a local build and the local build experiment flag is enabled.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly enhances App Hosting deployment by introducing support for local builds. Key changes include updating dependencies, adding a new ensureAppHostingServiceAgentRoles function to manage IAM permissions for the App Hosting service agent, and implementing splitEnvVars to categorize environment variables for build and runtime. The localBuild function now injects environment variables into process.env during the build process. The deployment flow has been updated to use .tar.gz archives for local builds, requiring the apphostinglocalbuilds experiment to be enabled. Firebase configuration is now automatically injected into local builds when an appId is present. The review comments highlight a consistent issue across multiple files where any is used in catch blocks, recommending the use of unknown with proper type checking to adhere to the style guide and improve type safety.
| } catch (e: any) { | ||
| if (e.message && !e.message.includes("doesn't exist")) { | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
The style guide prohibits using any as an escape hatch. Using e: any in a catch block is unsafe. Please use e: unknown and perform a type check before accessing its properties.
| } catch (e: any) { | |
| if (e.message && !e.message.includes("doesn't exist")) { | |
| throw e; | |
| } | |
| } | |
| } catch (e: unknown) { | |
| if (e instanceof Error && e.message && !e.message.includes("doesn't exist")) { | |
| throw e; | |
| } | |
| } |
References
- The repository style guide states to 'Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards.' Usinganyin a catch block without type checking is an example of such an escape hatch. (link)
| } catch (e) { | ||
| logLabeledWarning( | ||
| "apphosting", | ||
| `Unable to lookup details for backend ${cfg.backendId}. Firebase SDK autoinit will not be available.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The style guide prohibits using any, and implicit any in a catch block should be avoided. Please explicitly type the caught error as unknown.
| } catch (e) { | |
| logLabeledWarning( | |
| "apphosting", | |
| `Unable to lookup details for backend ${cfg.backendId}. Firebase SDK autoinit will not be available.`, | |
| ); | |
| } | |
| } catch (e: unknown) { | |
| logLabeledWarning( | |
| "apphosting", | |
| `Unable to lookup details for backend ${cfg.backendId}. Firebase SDK autoinit will not be available.`, | |
| ); | |
| } |
References
- The repository style guide states to 'Never use
anyorunknownas an escape hatch.' An untypedcatchclause results in an implicitanytype, which should be avoided in favor ofunknown. (link)
| } catch (err: any) { | ||
| if (err.code === "ENOENT") { | ||
| throw new FirebaseError(`Could not read directory "${rootDir}"`); | ||
| } | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
The style guide prohibits using any as an escape hatch. Please use err: unknown and perform a type check before accessing properties on the error object.
| } catch (err: any) { | |
| if (err.code === "ENOENT") { | |
| throw new FirebaseError(`Could not read directory "${rootDir}"`); | |
| } | |
| throw err; | |
| } | |
| } catch (err: unknown) { | |
| if ((err as NodeJS.ErrnoException)?.code === "ENOENT") { | |
| throw new FirebaseError(`Could not read directory "${rootDir}"`); | |
| } | |
| throw err; | |
| } |
References
- The repository style guide states to 'Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards.' Usinganyin a catch block without type checking is an example of such an escape hatch. (link)
4 participants
Description
Improves the ability to test changes locally and build firebase app hosting apps locally before deployment.
Scenarios Tested
Sample Commands