-
Notifications
You must be signed in to change notification settings - Fork 217
Add Firebase Data Connect v2 support #1727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Conversation
13cbf42 to
52a95a3
Compare
|
/gemini review |
There was a problem hiding this 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 support for Firebase Data Connect v2 with the onMutationExecuted trigger. The changes are well-structured, including the new provider, unit tests, and necessary package configuration updates. I've identified a few type safety issues in the new provider implementation concerning the handling of optional parameters, which could lead to runtime errors. I've provided suggestions to address these. Additionally, I've included a recommendation for a minor refactoring to enhance code clarity.
c933eab to
c3ecbbc
Compare
….operation gets populated
| @@ -0,0 +1,447 @@ | |||
| // The MIT License (MIT) | |||
| // | |||
| // Copyright (c) 2023 Firebase | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Copyright (c) 2023 Firebase | |
| // Copyright (c) 2025 Firebase |
| service.hasWildcards() | ||
| ? (eventFilterPathPatterns.service = service.getValue()) | ||
| : (eventFilters.service = service.getValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit* find this ternary operator a bit odd. I usually expect ternarary operator to return a value. For assignments, I might jus stick to classic if-statements
| Operation extends string = string | ||
| > extends EventHandlerOptions { | ||
| /** Firebase Data Connect service ID */ | ||
| service?: Service; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm a bit surprised that all these options are optional 🤔 should the all be required? what does not providing one mean? is it interpreted as wildcard or something else
Description
Adds basic FDC function trigger for onMutationExecuted
Includes unit tests. Also fixes a typo on an existing file
npm testpasses locallyInternal tracking bug: b/436623435
Code sample
Examples of how to use this
See go/fdc-eventarc-functions-sdk for more information