Skip to content

Conversation

@taeold
Copy link
Contributor

@taeold taeold commented Jul 14, 2025

feat: Add experimental file-based discovery via env var

Adds a new environment variable, FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH, to enable an experimental file-based function discovery mechanism.

  • If FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH=true, a temporary file path will be generated for the discovery manifest (functions.yaml).
  • If FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH is set to a path, that path will be used to store the discovery manifest.

This change also includes a refactoring of the discovery codebase to use the modern fs/promises API instead of util.promisify.

taeold and others added 6 commits July 11, 2025 08:09
- Add detectFromStdio() function to parse base64-encoded manifest from stderr
- Extract findFunctionsBinary() to avoid code duplication
- Add execAdmin() method for stdio discovery execution
- Modify discoverBuild() to use stdio when FIRESTORE_EMULATOR_HOST is set
- Stdio discovery avoids HTTP server overhead and improves reliability on resource-constrained CI/CD machines
- Maintains backward compatibility with existing HTTP discovery

This implementation requires firebase-functions SDK v6.3.2+ with stdio discovery support.
…T_OUTPUT_PATH

- Replace stdio XML parsing with file-based discovery mechanism
- Functions SDK writes manifest to path specified by FUNCTIONS_MANIFEST_OUTPUT_PATH env var
- CLI reads manifest from file path and cleans up after reading
- Preserves stderr capture for error reporting
- Updates discovery tests to use new env var instead of FUNCTIONS_DISCOVERY_MODE
- Maintains backward compatibility - uses HTTP discovery when env var not set
- Update all fixtures to use local firebase-functions from /Users/daniel/dev/firebase-functions
- Add runtime: nodejs22 to all firebase.json files
- Add stress-test fixture with 20 functions to test scalability
- Fix package.json syntax issues (trailing commas, empty engines)
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 @taeold, 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 an experimental, file-based function discovery mechanism for Firebase Functions. Instead of relying on an HTTP server to expose function metadata, this new approach allows the user's function code to dynamically generate a functions.yaml manifest file, which the Firebase CLI then reads to determine available functions. This change aims to provide a more robust and potentially faster discovery method, especially for complex projects or environments where HTTP-based discovery might be problematic.

Highlights

  • New Function Discovery Mechanism: I've added experimental support for a new function discovery method that relies on user code dynamically generating a functions.yaml manifest file. This provides an alternative to the existing HTTP-based discovery.
  • Environment Variable Activation: This new file-based discovery is activated by setting the FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH environment variable, which specifies the output location for the generated functions.yaml.
  • Refactored Discovery Logic: The core function discovery logic has been updated to prioritize static YAML discovery, then the new file-based discovery (if the environment variable is set), and finally fall back to the existing HTTP-based discovery.
  • Enhanced Test Suite: I've added a new 'stress-test' fixture to validate the discovery of a large number of functions and updated existing test fixtures to use the nodejs22 runtime and local firebase-functions SDK paths. The test suite now includes specific tests for the new file-based discovery method.
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 is currently in preview and 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 to provide feedback.

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

  1. 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.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 an experimental feature for discovering functions by dynamically generating a functions.yaml manifest file. The changes include the core logic for this new file-based discovery mechanism, updates to the Node.js runtime delegate to support it, and new tests. Please address the hardcoded paths, potential data loss, and code duplication.

taeold added 10 commits July 14, 2025 11:45
Adds a new environment variable, FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH, to enable an experimental file-based function discovery mechanism.

- If FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH=true, a temporary file path will be generated for the discovery manifest (functions.yaml).
- If FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH is set to a path, that path will be used to store the discovery manifest.

This change also includes a refactoring of the discovery codebase to use the modern fs/promises API instead of util.promisify.
Refactors the function discovery integration tests to use a shared helper function, reducing code duplication and improving maintainability.
Update all test fixtures to use the released firebase-functions v6.4.0
instead of local file references, now that the stdio discovery feature
is available in the published package.

Also fix Node engine requirement in yarn workspaces fixture to support Node 22.
- Use fs.mkdtemp instead of Date.now() to avoid race conditions in tests
- Add proper TypeScript type for childProcess parameter (ChildProcess)
- Replace promisify(fs.mkdtemp) with fs.promises.mkdtemp
- Replace promisify(fs.exists) with fs.promises.access
- Change discovery manifest log messages from logLabeledBullet to logger.debug
- Run npm format to fix code style issues
Remove unconditional file deletion logic that could delete user-specified
files. Temporary files in os.tmpdir() will be cleaned up by the OS.
@taeold
Copy link
Contributor Author

taeold commented Jul 15, 2025

/gemini reciew

@taeold
Copy link
Contributor Author

taeold commented Jul 15, 2025

/gemini review

@gemini-code-assist
Copy link
Contributor

Thanks for triggering the review, @taeold! I'm now performing a comprehensive code review of your pull request. Please note that your previous command /gemini reciew had a typo; the correct command is /gemini review.

I'll post my findings as review comments shortly.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 an experimental file-based discovery mechanism for Firebase Functions. I've identified a critical issue in the integration tests, a minor typing issue, and an opportunity to refactor duplicated code.

taeold and others added 7 commits July 15, 2025 14:35
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Fix test to use correct env var (FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH)
- Refactor detectFromOutputPath to poll for manifest file instead of waiting for process exit
- Extract common spawn logic to eliminate code duplication between execAdmin/serveAdmin
- Make execAdmin and serveAdmin function signatures consistent
- Improve error handling and support partial file writes

This makes file-based discovery more robust by allowing the SDK to write
the manifest at any time during execution, similar to HTTP discovery.
@taeold taeold marked this pull request as ready for review July 16, 2025 18:39
Copy link
Contributor

@joehan joehan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for adding integration test coverage

Reverted the polling implementation back to the simpler approach that waits
for process exit before reading the manifest file. The added complexity of
polling was not justified since:
- The SDK likely writes the file just before exiting anyway
- We still need the childProcess parameter
- The simpler approach is more predictable and easier to maintain
taeold added 2 commits July 16, 2025 15:00
- Replace || with ?? for nullish coalescing in discovery timeout checks
- Add comment explaining defensive config || {} check for external data
- Move stderr handling to spawnFunctionsProcess for consistency
- Refactor nested if-else to improve readability with early returns
- Add JSDoc comment explaining execAdmin's purpose
- Add TODO for future refactoring of nested promises in serveAdmin

All integration tests passing.
@taeold taeold enabled auto-merge (squash) July 16, 2025 22:02
The getFunctionDiscoveryTimeout() function returns 0 when no timeout
is set. Using ?? would pass through the 0 value causing immediate
timeouts. Using || ensures we fallback to the default timeout for
falsy values including 0.

Fixes unit test failure: "Timeout after 0"
@taeold taeold merged commit a02b978 into master Jul 16, 2025
48 of 50 checks passed
@taeold taeold deleted the feat/stdio-discovery branch July 16, 2025 22:24
@github-project-automation github-project-automation bot moved this from Approved [PR] to Done in [Cloud] Extensions + Functions Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants