Add V1-compatible getters for context and message #1800
Conversation
Summary of ChangesHello @shettyvarun268, 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 implements a crucial compatibility layer within the Firebase Functions v2 Pub/Sub trigger ( Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
|
/gemini review |
![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 introduces a compatibility layer for onMessagePublished to ease migration from v1, by adding context and message getters to the event object. The implementation correctly adds these getters using Object.defineProperty. The accompanying tests are thorough and cover both the new v1-compatible getters and ensure that standard v2 property access is unaffected.
My review includes suggestions to improve performance and maintainability:
- In
src/v2/providers/pubsub.ts, I've recommended memoizing the results of the new getters to avoid re-creating objects on every access. For themessagegetter, I've also suggested a refactor to reduce code duplication. - In
spec/v2/providers/pubsub.spec.ts, I've pointed out some code duplication in the new tests that could be refactored for better maintainability.
Overall, this is a great addition for improving developer experience during migration. The changes are well-implemented and tested.
There was a problem hiding this comment.
Code Review
This pull request introduces a v1-compatible API layer for v2 Pub/Sub onMessagePublished triggers by adding context and message getters to the event object. The implementation correctly uses getters for lazy evaluation and includes comprehensive tests for both the new compatibility layer and for ensuring no regressions in existing v2 functionality. My feedback focuses on improving code maintainability by reducing duplication in both the implementation and the tests.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable compatibility layer for developers migrating from v1 to v2 of the Firebase Functions SDK for Pub/Sub triggers. The addition of context and message getters on the CloudEvent object is a thoughtful approach to ease this transition. The implementation is clean, using Object.defineProperty to dynamically add the getters, which avoids polluting the core types, and includes caching for performance. The accompanying tests are thorough and cover both the new functionality and potential regressions. I've found one minor issue with a duplicated line of code, which I've commented on.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable v1-compatibility layer for onMessagePublished events, which will significantly ease migration for developers. The implementation correctly uses getters on the event object to provide context and message properties with a v1-like structure. The new tests are thorough, covering both the new v1-compatible getters and ensuring that standard v2 event properties remain unaffected. I've included a couple of suggestions to enhance type safety and the robustness of the caching mechanism within the new getters.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable compatibility layer for developers migrating from v1 to v2 of the Firebase Functions SDK by adding context and message getters to the onMessagePublished event. The implementation is efficient, using lazy-initialized getters and caching the results. The accompanying tests are thorough, covering both the new functionality and potential regressions.
My main feedback is to improve the type safety of this new API. Currently, the added properties are not reflected in the event type, which would force developers to use type assertions. I've left a detailed comment with a suggestion on how to introduce a specific event type to address this, which would significantly enhance the developer experience.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a helpful v1-compatibility layer for onMessagePublished events by adding context and message getters. The implementation is solid, using lazy-initialization with symbols for caching, and the tests are comprehensive.
My review includes a few suggestions to enhance the robustness and maintainability of this new layer:
- Ensuring type safety by throwing an error for malformed events instead of returning
undefinedwhere the type is non-nullable. - Improving the v1
messagecompatibility by conditionally adding theorderingKey. - Refactoring the inline message type into a named interface for better readability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable compatibility layer for developers migrating from v1 to v2 of the Firebase Functions SDK for Pub/Sub triggers. The implementation correctly adds context and message getters to the event object, and the use of Symbols for caching is a good practice. The accompanying tests are thorough and cover the new functionality well. I have a couple of suggestions to improve documentation clarity and ensure consistency in the serialization of the v1-compatible message object.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a v1-compatibility layer for the v2 onMessagePublished Pub/Sub trigger, which is a helpful addition for developers migrating from v1. The overall approach of adding context and message getters to the event object is sound. However, I've found a critical issue in the implementation of the context getter that breaks the intended v1-compatibility. It incorrectly uses the CloudEvent ID and time instead of the Pub/Sub message ID and publish time. I've provided a detailed comment with a suggested fix for both the implementation and the corresponding test.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable compatibility layer to the v2 onMessagePublished Pub/Sub trigger, simplifying migration from v1. The implementation adds context and message getters to the CloudEvent object, mimicking the v1 structure. This is achieved cleanly using Object.defineProperty and Symbol for caching, which is a great approach for performance and avoiding property collisions. The accompanying tests are thorough, covering the new getters and ensuring no regressions in existing v2 functionality. I have one suggestion to further improve the maintainability of the new implementation by reducing some code duplication.
brittanycho
left a comment
brittanycho
left a comment
There was a problem hiding this comment.
lgtm (assuming the other comment I left is resolved)
* Quick fix changelogs for #1800 pubsub compat * Fixing lint:prettier error
egilmorez
left a comment
egilmorez
left a comment
There was a problem hiding this comment.
Couple of minor formatting things for you thanks!
| } | ||
|
|
||
| /** | ||
| * A v1-compatible Pub/Sub Message. |
There was a problem hiding this comment.
Here and throughout, should we be capitalizing "message?" If it's a literal, yes, and then we need backticks as well; but I think "message" may be fine.
|
|
||
| // Add v1 compatible getters to the event object | ||
| /** | ||
| * Get the v1-compatible EventContext object. |
There was a problem hiding this comment.
This (esp if it gets parsed into reference docs) should be a literal, EventContext
3 participants
Please Disregard the previous PR: 1782. Will close that PR.
This PR introduces a compatibility layer to the v2 [onMessagePublished] Pub/Sub trigger to simplify migration for developers coming from the v1 Firebase Functions SDK.
Specifically, it adds two new getters directly to the [CloudEvent] object received by the function:
EventContext.messageId,publishTime, andorderingKey.Users should be able to use it as below:
import { onMessagePublished } from "firebase-functions/v2/pubsub";
// v1-style business logic
function handleMessage(message, context) {
console.log(message.json, context.eventId);
}
export const myV2Function = onMessagePublished("my-topic", (event) => {
const { message, context } = event; // Easily get v1-compatible objects
handleMessage(message, context);
});