-
Notifications
You must be signed in to change notification settings - Fork 59
Support Params as inputs in eventTrigger #168
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import { CloudEvent, CloudFunction } from 'firebase-functions/v2'; | ||
import { Expression } from 'firebase-functions/v2/params'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aside - we are planning to change the import path of Expression from I wonder if there are more "elegant' way to import the type to avoid having this problem 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When is the change going to occur? 🙃 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. v4 sdk! |
||
|
||
export const APP_ID = '__APP_ID__'; | ||
export const PROJECT_ID = '42'; | ||
|
@@ -10,7 +11,7 @@ export function getEventType(cloudFunction: CloudFunction<any>): string { | |
|
||
export function getEventFilters( | ||
cloudFunction: CloudFunction<any> | ||
): Record<string, string> { | ||
): Record<string, string | Expression<string>> { | ||
return cloudFunction?.__endpoint?.eventTrigger?.eventFilters || {}; | ||
} | ||
|
||
|
@@ -27,6 +28,15 @@ export function getBaseCloudEvent<EventType extends CloudEvent<unknown>>( | |
} as EventType; | ||
} | ||
|
||
export function resolveStringExpression( | ||
stringOrExpression: string | Expression<string> | ||
) { | ||
if (typeof stringOrExpression === 'string') { | ||
return stringOrExpression; | ||
} | ||
return stringOrExpression?.value(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably double check with @Berlioz - but I thought it is very possible that this to return empty string. I'm wondering if we should think more about supporting Params firebase-functions. Params are a way to specify "value of this is going to be determined at deploy time" - so it is probably going to be undefined at test time unless there is some special override mechanism. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem I was working against was There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests show a scenario where its populated at runtime too |
||
} | ||
|
||
function makeEventId(): string { | ||
return ( | ||
Math.random() | ||
|
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.
Should we update the peer-depdency? It looks like our package won't work unless you are using firebase-functions 3.23 or above now since we explicitly try to import the Expression type in v2/params.
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.
You're extremely right!!!
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.
That probably means we need to do a minor or major bump then... WDYT?