-
Notifications
You must be signed in to change notification settings - Fork 216
Add dotguides #1736
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
Add dotguides #1736
Conversation
Summary of ChangesHello @jhuleatt, 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 set of comprehensive guides aimed at streamlining LLM-assisted development with Firebase Functions. These guides cover the entire lifecycle, from initial project setup and best practices for using the latest SDK features, to a detailed migration path for upgrading existing 1st generation functions to the more modern and efficient 2nd generation. The goal is to provide clear, actionable documentation to enhance developer productivity and ensure secure, scalable function deployments. Highlights
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
|
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 adds guides for LLM-assisted development. The guides are well-structured and provide useful information for setting up, upgrading, and using Firebase Functions. I've found a few inconsistencies and typos in the code examples within the markdown files. My suggestions aim to correct these issues and improve the clarity and accuracy of the documentation.
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.
I think this can be really powerful, but we need to be careful. Maybe the goal of /upgrade
should be to upgrade a single function at a time and the guide should be written in that way - like step 1 find v1 functions step 2 ask user which function to upgrade and then it has the guidance to do that.
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.
Yeah that does sound safer. I've changed step 1 to find and prompt for a single function to migrate.
Find all 1st-gen functions in the directory. 1st-gen functions used a namespaced API like this: | ||
|
||
**Before (1st Gen):** | ||
|
||
```typescript | ||
import * as functions from "firebase-functions"; | ||
|
||
export const webhook = functions.https.onRequest((request, response) => { | ||
// ... | ||
}); | ||
``` | ||
|
||
Sometimes, they'll explicitly import from the `firebase-functions/v1` subpackage, but not always. |
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.
@taeold this seems like a cool opportunity for an mcp tool
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.
yeah!
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.
MCP tools for something very niche like "find all v1 functions" are a bit of a tough sell for me, but what you can do is pre-specify shell commands that will get the info you want. For example a find
or grep
or sed
command that helps it find all the files it needs to look at.
Find all 1st-gen functions in the directory. 1st-gen functions used a namespaced API like this: | ||
|
||
**Before (1st Gen):** | ||
|
||
```typescript | ||
import * as functions from "firebase-functions"; | ||
|
||
export const webhook = functions.https.onRequest((request, response) => { | ||
// ... | ||
}); | ||
``` | ||
|
||
Sometimes, they'll explicitly import from the `firebase-functions/v1` subpackage, but not always. |
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.
yeah!
Co-authored-by: Daniel Lee <[email protected]>
…ons into dotguides-pilot
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.
Here's some thoughts Jeff, sorry it's closing in on 4pm on a Friday :)
.guides/config.json
Outdated
@@ -0,0 +1,9 @@ | |||
{ | |||
"description": "Use this library to build serverless functions for event triggers and HTTP using Firebase and Cloud Run Functions", |
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.
Are we saying "Firebase and Cloud Run Functions" very deliberately, for a good reason?
If not, I'd avoid mention of a separate product, and just go with "using Cloud Functions for Firebase."
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.
fixed
.guides/setup.md
Outdated
@@ -0,0 +1,69 @@ | |||
# Cloud Functions for Firebase setup guide | |||
|
|||
This guide provides a step-by-step process for setting up a new Firebase Functions project, tailored for a coding agent. |
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.
We should watch out for this non-official naming . . . suggest just "Cloud Functions."
Also suggest "coding agents."
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.
fixed
.guides/setup.md
Outdated
|
||
**Key points for the agent:** | ||
|
||
- Always import from `firebase-functions/*` for new functions. |
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.
Do we want to call out the Auth and Analytics exceptions? "Always" is a strong word to a robot.
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.
I'll remove this bullet point, I think we have enough examples in here that it isn't needed.
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.
plus, everything is under "firebase-functions/*" so it isn't really saying anything
.guides/setup.md
Outdated
|
||
Use the Firebase Emulators to test your function locally before deploying. | ||
|
||
Tell the user to run the following command in a separate terminal window to start the emulators: |
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.
Would "developer" be a better term than user?
Just thinking of how literal-minded an agent might be.
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.
I've updated to "human" #1736 (comment)
.guides/upgrade.md
Outdated
> 3. Deploy it alongside the old 1st gen function. | ||
> 4. Gradually introduce traffic to the new function (e.g., via client-side changes or by calling it from the 1st gen function). | ||
> 5. Add back the original `minInstances` and `maxInstances` settings to the 2nd gen function. | ||
> 6. Once you are confident, you can delete the 1st gen function. |
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.
By the same token, I wonder how an agent will interpret its level of confidence . . . we may need to be more specific/ less emotional :).
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.
fixed
.guides/usage.md
Outdated
- Always use 2nd-gen functions for new development. | ||
- Use 1st-gen functions _only_ for Analytics and basic Auth triggers. | ||
- Use `firebase-functions` SDK version 6.0.0 and above | ||
- Use top-level imports (e.g., `firebase-functions/https`). These are 2nd gen by default. |
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.
Do we need to be clear that Auth and analytics are going to be exceptions to this rule RE top-level imports?
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.
updated
.guides/usage.md
Outdated
}); | ||
``` | ||
|
||
When you deploy a function with `secrets`, the CLI will prompt you to enter the secret's value. Alternatively, you can instruct the user to set the secret using the Firebase CLI command: |
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.
Deployment question again -- is the agent doing the deploying? If so, "you" is fine (agents are the New You!! :)
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.
simplified this
README.md
Outdated
- Read the full API reference: https://firebase.google.com/docs/reference/functions/ | ||
- Browse some examples: https://github.com/firebase/functions-samples | ||
- [Start with the quickstart](https://firebase.google.com/docs/functions/write-firebase-functions) | ||
- [Go through the guide](https://firebase.google.com/docs/functions/) |
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.
Should we point them to functions/get-started?
If our aim is more general, let's pluralize "guides"
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.
fixed
README.md
Outdated
- Browse some examples: https://github.com/firebase/functions-samples | ||
- [Start with the quickstart](https://firebase.google.com/docs/functions/write-firebase-functions) | ||
- [Go through the guide](https://firebase.google.com/docs/functions/) | ||
- [Read the full API reference](https://firebase.google.com/docs/reference/functions/) |
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.
This reference link goes to a top-level "nav" page that includes 1st gen. Do we want instead to go directly to https://firebase.google.com/docs/reference/functions/2nd-gen/node/firebase-functions?"
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.
updated
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.
Thanks Jeff!!
Description
Adds guides for LLM-assisted development