Skip to content

Conversation

@taeold
Copy link
Contributor

@taeold taeold commented Nov 24, 2025

This PR removes the obsolete parseTriggers.ts and related files (triggerParser.js, extractTriggers.js), which were used as a fallback for parsing function triggers in older versions of the Firebase Functions SDK.

This is part of a major version update and drops support for SDK versions older than 3.20.0.

@gemini-code-assist
Copy link
Contributor

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 significantly refactors the function trigger parsing mechanism by removing an obsolete fallback that supported older versions of the Firebase Functions SDK. By dropping support for SDK versions prior to 3.20.0, the codebase is streamlined, and a more robust error-handling approach is implemented for incompatible SDK versions. This change ensures that the system operates with modern SDK features and reduces technical debt.

Highlights

  • Removal of Obsolete Trigger Parsing Fallback: The parseTriggers.ts module and its associated files (triggerParser.js, extractTriggers.js) have been completely removed, as they were part of an obsolete fallback mechanism for parsing function triggers.
  • Enforcement of Minimum SDK Version: The system now explicitly requires firebase-functions SDK version 3.20.0 or newer. Instead of falling back to old parsing logic, an error is now thrown if an older SDK version is detected.
  • Codebase Simplification: Eliminating the old trigger parsing logic and related files reduces codebase complexity and maintenance overhead, focusing on modern SDK features.
  • Updated Build Configuration: The webpack.common.js configuration was cleaned up by removing commented-out references to copying uncompiled JavaScript files that were part of the obsolete trigger parsing system.
  • New Test Coverage: A new test case was added to index.spec.ts to ensure proper error handling when an unsupported firebase-functions SDK version is detected during the build discovery process.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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 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

  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 effectively removes the obsolete parseTriggers fallback mechanism for older Firebase Functions SDK versions. The changes are clean and focused, including the removal of related files and tests. A new test has been added to ensure that an error is thrown for unsupported SDK versions, which is a great addition. My only concern is a small, potentially out-of-scope change in the webpack configuration that should be clarified.

@taeold taeold force-pushed the feat/remove-parsetriggers branch from a60738d to e8ef72d Compare November 24, 2025 23:05
@taeold taeold force-pushed the feat/remove-parsetriggers branch from e8ef72d to bf66853 Compare November 24, 2025 23:21
@taeold
Copy link
Contributor Author

taeold commented Nov 27, 2025

/gemini review

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 effectively removes the obsolete parseTriggers fallback mechanism, which simplifies the codebase by deleting several files and their associated tests. The logic is correctly updated to enforce a minimum Firebase Functions SDK version by throwing an error instead of falling back, which is a sound approach for a major version update. The changes are clean and well-tested. I have one minor suggestion to improve code readability.

Comment on lines 294 to 312
if (!semver.valid(this.sdkVersion)) {
logger.debug(
`Could not parse firebase-functions version '${this.sdkVersion}' into semver. Falling back to parseTriggers.`,
);
return parseTriggers.discoverBuild(this.projectId, this.sourceDir, this.runtime, config, env);
}
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION)) {
logLabeledWarning(
"functions",
`You are using an old version of firebase-functions SDK (${this.sdkVersion}). ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION}`,
);
return parseTriggers.discoverBuild(this.projectId, this.sourceDir, this.runtime, config, env);
}
// Perform a check for the minimum SDK version that added annotation support for the `Build.extensions` property
// and log to the user explaining why they need to upgrade their version.
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES)) {
logLabeledBullet(
"functions",
`You are using a version of firebase-functions SDK (${this.sdkVersion}) that does not have support for the newest Firebase Extensions features. ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES} to use them correctly`,
);
logger.debug(`Could not parse firebase-functions version '${this.sdkVersion}' into semver.`);
} else {
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION)) {
throw new FirebaseError(
`You are using an old version of firebase-functions SDK (${this.sdkVersion}). ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION}`,
);
}
// Perform a check for the minimum SDK version that added annotation support for the `Build.extensions` property
// and log to the user explaining why they need to upgrade their version.
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES)) {
logLabeledBullet(
"functions",
`You are using a version of firebase-functions SDK (${this.sdkVersion}) that does not have support for the newest Firebase Extensions features. ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES} to use them correctly`,
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

For better readability, you could invert the if/else to handle the valid semver case first. This avoids nesting the main logic within an else block and follows a more common pattern of handling the success path before the error/edge case path.

Suggested change
if (!semver.valid(this.sdkVersion)) {
logger.debug(
`Could not parse firebase-functions version '${this.sdkVersion}' into semver. Falling back to parseTriggers.`,
);
return parseTriggers.discoverBuild(this.projectId, this.sourceDir, this.runtime, config, env);
}
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION)) {
logLabeledWarning(
"functions",
`You are using an old version of firebase-functions SDK (${this.sdkVersion}). ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION}`,
);
return parseTriggers.discoverBuild(this.projectId, this.sourceDir, this.runtime, config, env);
}
// Perform a check for the minimum SDK version that added annotation support for the `Build.extensions` property
// and log to the user explaining why they need to upgrade their version.
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES)) {
logLabeledBullet(
"functions",
`You are using a version of firebase-functions SDK (${this.sdkVersion}) that does not have support for the newest Firebase Extensions features. ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES} to use them correctly`,
);
logger.debug(`Could not parse firebase-functions version '${this.sdkVersion}' into semver.`);
} else {
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION)) {
throw new FirebaseError(
`You are using an old version of firebase-functions SDK (${this.sdkVersion}). ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION}`,
);
}
// Perform a check for the minimum SDK version that added annotation support for the `Build.extensions` property
// and log to the user explaining why they need to upgrade their version.
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES)) {
logLabeledBullet(
"functions",
`You are using a version of firebase-functions SDK (${this.sdkVersion}) that does not have support for the newest Firebase Extensions features. ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES} to use them correctly`,
);
}
}
if (semver.valid(this.sdkVersion)) {
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION)) {
throw new FirebaseError(
`You are using an old version of firebase-functions SDK (${this.sdkVersion}). ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION}`,
);
}
// Perform a check for the minimum SDK version that added annotation support for the `Build.extensions` property
// and log to the user explaining why they need to upgrade their version.
if (semver.lt(this.sdkVersion, MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES)) {
logLabeledBullet(
"functions",
`You are using a version of firebase-functions SDK (${this.sdkVersion}) that does not have support for the newest Firebase Extensions features. ` +
`Please update firebase-functions SDK to >=${MIN_FUNCTIONS_SDK_VERSION_FOR_EXTENSIONS_FEATURES} to use them correctly`,
);
}
} else {
logger.debug(`Could not parse firebase-functions version '${this.sdkVersion}' into semver.`);
}

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.

1 participant