-
Notifications
You must be signed in to change notification settings - Fork 211
Specific interface for provider enhancement (2/3) #1022
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
Conversation
handler: (event: BillingEvent<T>) => any | Promise<any> | ||
): CloudFunction<FirebaseAlertData<T>> { | ||
if (typeof optsOrHandler === 'function') { | ||
handler = optsOrHandler as (event: BillingEvent<T>) => any | Promise<any>; |
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.
Woo (event: BillingEvent<T>) => any | Promise<any>;
is used thrice! This meets the threshold for DRY for me:
type BillingEventHandler<T> = (event: BillingEvent<T>) => any | Promise<any>;
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.
Oh that looks way better, thanks for the suggestion!
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.
@taeold Removed based on typing feedback below
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.
🔥 Great stuff.
src/v2/providers/alerts/billing.ts
Outdated
export const automatedPlanUpdateAlert = 'billing.automatedPlanUpdate'; | ||
|
||
/** @internal */ | ||
type BillingEventHandler<T> = (event: BillingEvent<T>) => any | Promise<any>; |
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.
This typedef makes code complete worse (it asks for a BillingEventHandler instead of a function signature)
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.
Oh shoot, we should probably change it back for better usability then, 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.
I wanted to test out the automcomplete and see what shows, but I couldn't get to any of the billing handlers at all.
@colerogers Can you double check if this is working for you?
npm run build:pack
- Install the generated tgz locally on your function
- Try to get import to work.
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.
In fact, I couldn't even get lib/v2/providers/billing.js
to show up. what gives?
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.
@taeold I was able to get it to work, just pushed an update. Seems like we can't rely on the package.json export path alone, we need to also export from the root index.ts. I think there was a comment about exporting on one of the other PRs and I ended up changing it on all of them
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.
@inlined Removed BillingEventHandler type from the latest commit
* breaking out general interface * cleaning up exports * removing comments & references to specific interfaces * format * jsdoc comments * address pr comments * added param comment * addressing comments * Specific interface for provider enhancement (1/3) (#1021) * adding in app distro changes * removing comments & addings package refs * jsdoc comments * fix comments * adding periods to doc strings * fix wording * adding import/export statement * Specific interface for provider enhancement (2/3) (#1022) * adding in billing changes * removing comments & adding package refs * jsdoc comments * addressing pr comments * change handler doc string * changing to BillingEventHandler type * remove BillingEventHandler type * Specific interface for provider enhancement (3/3) (#1023) * adding in crashlytics changes * comments & adding package refs * address comments and make doc strings better * add opts.retry to event trigger
* breaking out general interface * cleaning up exports * removing comments & references to specific interfaces * format * jsdoc comments * address pr comments * added param comment * addressing comments * Specific interface for provider enhancement (1/3) (#1021) * adding in app distro changes * removing comments & addings package refs * jsdoc comments * fix comments * adding periods to doc strings * fix wording * adding import/export statement * Specific interface for provider enhancement (2/3) (#1022) * adding in billing changes * removing comments & adding package refs * jsdoc comments * addressing pr comments * change handler doc string * changing to BillingEventHandler type * remove BillingEventHandler type * Specific interface for provider enhancement (3/3) (#1023) * adding in crashlytics changes * comments & adding package refs * address comments and make doc strings better * add opts.retry to event trigger
Change for the the specific billing interface. Exposes
onPlanUpdatePublished
&onAutomatedPlanUpdatePublished
from thev2/alerts/billing
namespaceEx ~