refactor(kits): read the kit instance id from FIREBASE_KIT_INSTANCE_ID - #3122
refactor(kits): read the kit instance id from FIREBASE_KIT_INSTANCE_ID#3122cabljac wants to merge 2 commits into
Conversation
![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 updates multiple Firebase kits (bigquery-firestore-export, firestore-incremental-capture, and firestore-vector-search) to retrieve the instance ID from the FIREBASE_KIT_INSTANCE_ID environment variable injected by the Firebase CLI (version 15.27.0 or later), rather than requiring it as a configuration parameter (INSTANCE_ID). This change simplifies configuration and prevents manual synchronization errors. Feedback suggests optimizing the code by reusing the module-level instanceId constant instead of calling instanceIdFromEnv() redundantly at runtime in both bigquery-firestore-export and firestore-vector-search configurations.
| bigqueryDatasetLocation: params.bigqueryDatasetLocation.value(), | ||
| projectId: projectID.value(), | ||
| instanceId: resolvedInstanceId, | ||
| instanceId: instanceIdFromEnv(), |
There was a problem hiding this comment.
Since instanceId is already resolved at module load time (line 45), calling instanceIdFromEnv() again here is redundant. You can directly use the instanceId constant. Note that if you apply this change, the test throws at runtime when FIREBASE_KIT_INSTANCE_ID is missing in config.test.ts will become obsolete and should be removed, as the import-time check already guarantees the variable is present when the module is loaded.
| instanceId: instanceIdFromEnv(), | |
| instanceId, |
| region: process.env.FUNCTION_REGION, | ||
| projectId: projectID.value(), | ||
| instanceId: params.instanceId.value(), | ||
| instanceId: instanceIdFromEnv(), |
There was a problem hiding this comment.
Since instanceId is already resolved at module load time (line 66), calling instanceIdFromEnv() again here is redundant. You can directly use the instanceId constant. Note that if you apply this change, the test throws at runtime when FIREBASE_KIT_INSTANCE_ID is missing in config.test.ts will become obsolete and should be removed, as the import-time check already guarantees the variable is present when the module is loaded.
| instanceId: instanceIdFromEnv(), | |
| instanceId, |
firebase-tools >= 15.27.0 injects FIREBASE_KIT_INSTANCE_ID for every kit instance at discovery, deploy, emulator and serve time. The params machinery never sees injected values and the FIREBASE_ prefix is reserved in .env files, so bigquery-firestore-export, firestore-incremental-capture and firestore-vector-search now read the variable directly instead of declaring a user-supplied INSTANCE_ID param, matching delete-user-data. Instance-scoped defaults (the Pub/Sub topic, the query trigger document path) are derived from the variable at import, so an unsupported CLI fails the discovery pass with an error naming the version floor rather than freezing kit-undefined-* names into the manifest. configFromEnv throws the same error at runtime. The repo-wide CLI floor lives in kits/README.md; each kit README repeats it next to its kit stanza and drops INSTANCE_ID from its params table.
The ./lib entry re-exports configFromEnv from config.ts, and the README promises it reads no environment at load so users can import it from their own triggers. Resolving FIREBASE_KIT_INSTANCE_ID at module scope in config.ts broke that. The query trigger path is now derived in index.ts, the deploy-only entry, so discovery still fails loudly on an unsupported CLI while lib.ts loads without the variable. Tests pin both. Also asserts on declaredParams in the bigquery-firestore-export test, narrows the kits README wording to kits that use the instance id, and rewraps a long README line.
9468548 to
6ae6eeb
Compare
1 participant
firebase-tools >= 15.27.0 injects
FIREBASE_KIT_INSTANCE_IDfor every kit instance, but bigquery-firestore-export, firestore-incremental-capture and firestore-vector-search still declaredINSTANCE_IDas a user-supplied param. The params machinery never sees injected values andFIREBASE_is reserved in.env, so this follows the #3060 pattern: a plainprocess.envread, the param removed, andconfigFromEnvthrowing an error that names the CLI floor when the variable is missing.Where a kit derives a discovery-time value from the id (the bigquery Pub/Sub topic default, the vector-search query trigger path) the read happens when the deploy entry loads, so an unsupported CLI fails discovery instead of freezing
kit-undefined-*names into the manifest. The vector-search./libentry still loads without the variable, as its README promises; a test pins that.The repo-wide CLI floor now lives in
kits/README.md; each kit README repeats it, dropsINSTANCE_IDfrom its params table, and gains a changelog entry.Tests: bigquery-firestore-export 79, firestore-incremental-capture 75, firestore-vector-search 120, plus
tsc -band prettier. Not verified: a live deploy or emulator run against 15.27.0.Fixes #3063