Fix incorrect FAL event type. Add regression exception & test#10268
Fix incorrect FAL event type. Add regression exception & test#10268
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the AI Logic event names to use the new 'google.firebase.ailogic.v1' prefix and adds validation logic to detect and reject unrecognized blocking trigger types. I have reviewed the changes and suggest a minor refactoring in the validation logic to reduce nesting by reusing the service object, which aligns with the repository's style guide.
src/deploy/functions/validate.ts
Outdated
| if (backend.isBlockingTriggered(ep)) { | ||
| const service = serviceForEndpoint(ep); | ||
| if (service.name === "noop") { | ||
| throw new FirebaseError( | ||
| `Unrecognized blocking trigger type: ${ep.blockingTrigger.eventType}. Please update your CLI with ${clc.bold("npm install -g firebase-tools@latest")}.`, | ||
| { exit: 1 }, | ||
| ); | ||
| } | ||
| } | ||
| serviceForEndpoint(ep).validateTrigger(ep, wantBackend); |
There was a problem hiding this comment.
The serviceForEndpoint(ep) function is called twice for blocking triggers. By calling it once and reusing the result, we can simplify the logic and reduce nesting, which aligns with the repository's style guide recommendation to keep code flat and efficient.
const service = serviceForEndpoint(ep);
if (backend.isBlockingTriggered(ep) && service.name === "noop") {
throw new FirebaseError(
`Unrecognized blocking trigger type: ${ep.blockingTrigger.eventType}. Please update your CLI with ${clc.bold("npm install -g firebase-tools@latest")}.`,
{ exit: 1 },
);
}
service.validateTrigger(ep, wantBackend);References
- Reduce nesting as much as possible. Code should avoid unnecessarily deep nesting or long periods of nesting. (link)
shettyvarun268
left a comment
There was a problem hiding this comment.
LGTM!
The nit about the double calling in validate.ts might be worth looking into.
For some reason, the CLI was using the wrong event type for AI Logic triggers. Fixed, and added a validator to throw & recommending upgrading the CLI if the blocking event type isn't known because it was maddening to figure out why these triggers were showing up as HTTP triggers (with blocking event types, we do the registration so there is no use for a fallthrough like to eventarc)
The redefinition of the event type constants was necessary to break circular logic. Maybe I should break down and put this all in services? The pattern should use some looking at in a future review.