-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat(billing): Add subscription hook #90432
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
| >; | ||
| }; | ||
|
|
||
| type Subscription = { |
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.
How come this type is diff from
sentry/static/gsApp/types/index.tsx
Lines 243 to 247 in d923587
| export type Subscription = { | |
| accountBalance: number; | |
| billingInterval: 'monthly' | 'annual'; | |
| // billingPeriod varies between 1-12 months. if you're looking for the monthly usage interval, use onDemandPeriodStart | |
| billingPeriodEnd: string; |
And relatedly, how come we don't import that type instead?
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.
We can't import anything from gsApp or gsAdmin outside of them: #87666
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 didn't copy the whole type only because the other fields are not currently used anywhere.
As an alternative, I can make this into a more specific hook (ie. useCurrentPlan), similar to what we do for useGetMaxRetentionDays
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.
Gotcha, good to know!
I'm on team specific hooks, though looking at the hook itself, there's nothing that actually does the response filtering on it atm right? Or am I missing something
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.
yeah you're correct
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 that case we're kinda stuck between a rock and a hard place, because the current typing is disingenuous to what the hook is actually doing, but writing out the full type again in this place also just opens us up to a similar thing to that DataCategory DataCategories thing.
It's a little funky to me that gsapp and gsadmin don't have the ability to also pull from a shared set of interfaces somewhere rather than redefining all these things.
I'll leave it up to you since ultimately it doesn't really matter, and I don't wanna make more work for ya
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.
yeah I agree with where you're coming from.
I think I'll go ahead and replace this PR with a version that uses a hook which renders the message (rather than a hook which returns the subscription data and using that to render the message). This is a pattern you can see with a bunch of billing CTAs throughout the app that don't live in gsApp or gsAdmin (ie. ProductUnavailableCTA, CronsBillingBanner, etc.)
lmk what you think! i'd also like to ask our FE folks a little more about that import linting rule (ie. is it an artifact of where the billing FE code used to live?)
Registers the
useSubscriptionhook so it may be used outside ofgsAppandgsAdmin+ corrects budget term in crons processing error component.