Conversation
|
I have an entirely different skill on migrating from configs to params. Maybe we should not even mention config except as instructions to always use params instead? Can you ping me tomorrow about this? |
@inlined, in the interest of getting this out in the hands of developers, want to either (1) keep |
|
@inlined I've removed |
egilmorez
left a comment
There was a problem hiding this comment.
Some thoughts for you Jeff, thanks!
inlined
left a comment
There was a problem hiding this comment.
Should we teach the model that --functions is used when only emulating functions? A lot of functions depend on event sources and I almost never run only --functions.
We might also want to add the genkit UI in the npx concurrently call
lahirumaramba
left a comment
There was a problem hiding this comment.
Thank you, Jeff! Admin SDK sections LGTM!
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a new skill for Cloud Functions for Firebase, including guides for Node.js and Python. The documentation is a good start, but there are several critical and high-severity issues in the provided code examples and setup instructions. The Node.js example incorrectly mixes v1 and v2 function APIs, which will not work. The Python example has a bug in how it sets runtime options and also contains confusing, unused code. Additionally, some of the general guidance on SDK versions and import paths is inaccurate. I've provided suggestions to correct these issues to ensure the guides are accurate and functional.
| import { setGlobalOptions, onInit } from "firebase-functions"; | ||
| import { onRequest } from "firebase-functions/https"; | ||
| import { onDocumentCreated } from "firebase-functions/firestore"; | ||
| import * as logger from "firebase-functions/logger"; | ||
| import { defineString, defineInt } from "firebase-functions/params"; | ||
|
|
||
| import { initializeApp } from "firebase-admin/app"; | ||
|
|
||
| // Configurable parameters | ||
| const scaleLimit = defineInt("MAX_INSTANCES", { default: 1 }); | ||
| const greeting = defineString("GREETING", { default: "Hello" }); | ||
|
|
||
| onInit(() => { | ||
| initializeApp(); | ||
| setGlobalOptions({ maxInstances: scaleLimit }); | ||
| }); | ||
|
|
||
| export const helloWorld = onRequest(async (request, response) => { | ||
| logger.info("Request received!", request); | ||
| response.send(`${greeting.value()} from Firebase!`); | ||
| }); | ||
|
|
||
| export const newDoc = onDocumentCreated( | ||
| "/words/{wordId}", | ||
| async (event) => { | ||
| logger.info("New word!", event.data?.data()); | ||
| } | ||
| ); |
There was a problem hiding this comment.
The provided code snippet for a 2nd-gen function is incorrect as it mixes APIs from both v1 and v2 of Cloud Functions, which will lead to errors. Specifically:
- Imports for
onRequestandonDocumentCreatedshould be fromfirebase-functions/v2/...subpaths for 2nd-gen functions. - The
onInitlifecycle hook is for v1 functions and should not be used. setGlobalOptionsshould be imported fromfirebase-functions/v2and called at the top level.- Logging the entire
requestobject can be a security risk and produce noisy logs. It's better to log specific, non-sensitive properties.
Here is a corrected version of the snippet that properly implements 2nd-gen functions:
import { onRequest } from "firebase-functions/v2/https";
import { onDocumentCreated } from "firebase-functions/v2/firestore";
import * as logger from "firebase-functions/logger";
import { defineString, defineInt } from "firebase-functions/params";
import { initializeApp } from "firebase-admin/app";
import { setGlobalOptions } from "firebase-functions/v2";
initializeApp();
// Configurable parameters
const scaleLimit = defineInt("MAX_INSTANCES", { default: 1 });
const greeting = defineString("GREETING", { default: "Hello" });
setGlobalOptions({ maxInstances: scaleLimit });
export const helloWorld = onRequest(async (request, response) => {
logger.info("Request received!", { body: request.body });
response.send(`${greeting.value()} from Firebase!`);
});
export const newDoc = onDocumentCreated(
"/words/{wordId}",
async (event) => {
logger.info("New word!", event.data?.data());
}
);There was a problem hiding this comment.
What is your source for that information? The default import has been 2nd gen for a while now, so I want to correct the source you're using.
| admin_app = None | ||
|
|
||
| @init | ||
| def initialize(): | ||
| options.set_global_options(max_instances=SCALE_LIMIT) | ||
|
|
||
| global admin_app | ||
| admin_app = initialize_app() |
There was a problem hiding this comment.
There are a couple of issues in the initialize function:
- The
admin_appglobal variable is assigned but never used, which is confusing. Callinginitialize_app()is sufficient as it sets up the default app instance. - The
SCALE_LIMITparameter is anIntParamobject. To use its integer value formax_instances, you must call the.value()method. Passing the object directly will cause a runtime error.
| admin_app = None | |
| @init | |
| def initialize(): | |
| options.set_global_options(max_instances=SCALE_LIMIT) | |
| global admin_app | |
| admin_app = initialize_app() | |
| @init | |
| def initialize(): | |
| options.set_global_options(max_instances=SCALE_LIMIT.value()) | |
| initialize_app() |
| admin_app = initialize_app() | ||
|
|
||
| @https_fn.on_request( | ||
| cors=options.CorsOptions(cors_origins="*", cors_methods=["get", "post"]) |
There was a problem hiding this comment.
Using a wildcard * for cors_origins allows any origin to access your function, which can be a security risk. For production environments, it is strongly recommended to restrict this to a specific list of allowed domains. For a guide, it's better to show a more secure example or add a note about this.
| cors=options.CorsOptions(cors_origins="*", cors_methods=["get", "post"]) | |
| cors=options.CorsOptions(cors_origins=["https://your-web-app.com"], cors_methods=["get", "post"]) |
joehan
left a comment
There was a problem hiding this comment.
Some nits, but this looks very good to me!
| ); | ||
| ``` | ||
|
|
||
| **Key points for the agent:** |
There was a problem hiding this comment.
| **Key points for the agent:** | |
| **Key points:** |
Its all for the agent
| ); | ||
| ``` | ||
|
|
||
| **Key points for the agent:** |
There was a problem hiding this comment.
| **Key points for the agent:** | |
| **Key points:** |
Its all for the agent
Add skills for Cloud Functions for Firebase
Fixes #9