-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[x-license] Add support for plan version #13459
[x-license] Add support for plan version #13459
Conversation
Deploy preview: https://deploy-preview-13459--material-ui-x.netlify.app/ |
This reverts commit 190a18e.
packages/x-license/src/useLicenseVerifier/useLicenseVerifier.ts
Outdated
Show resolved
Hide resolved
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.
The logic seems good 👌
Thanks for taking care of it
Co-authored-by: Andrew Cherniavskii <[email protected]> Signed-off-by: Michel Engelen <[email protected]>
and changed the licenseError for the product scope
|
@michelengelen I think we have also conformance test on range pickers. |
I would have liked to decide clarify the status here before merging: #13459 (comment) |
Oh, I thought with @joserodolfofreitas giving his 👍🏼 it was resolved... sry for that. I did however open a new PR #13568 where we can adjust this. |
I'm talking about the name of the status we use mostly, the error message is nice I think |
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.
These changes make sense to me at a high level, great to have them 👍
case LICENSE_STATUS.OutOfScope: | ||
return 'MUI X License key plan mismatch'; | ||
case LICENSE_STATUS.ProductNotCovered: | ||
return 'MUI X Product not covered by plan'; |
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.
It feels like these two errors OutOfScope
+ ProductNotCovered
are the same error. Should we merge them? As for the need to show different error messages, I think this should be done through the meta data.
If we want to keep them distinct, I would make their name match, e.g.
OutOfScope -> PlanScopeNotCovered
ProductNotCovered -> ProductScopeNotCovered
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 advocated to change ProductNotCovered
into a more specific NotAvailableInInitialProPlan
in #13568, so that we can display a more precise error in the doc.
We could indeed rename OutOfScope
which is super broad right now.
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.
NotAvailableInInitialProPlan
so that we can display a more precise error in the doc.
There will be more cases like this in the future of having a paid for one of the MUI X plans, and trying to use a different plan. I don't see creating a new error type each time scaling.
So It feels like this should be one error type and use the metadata to have a precise error in the console.
For the docs, I agree, I can see why we need to separate the cases: (a.) buying pro and using pro without working is a different confusion source than (b.) buying pro and trying to use premium.
But anyway, no strong push on my end, more of a feeling that in the code, it feels strange.
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 don't see creating a new error type each time scaling.
The status is only used to do some sort of switch and display a warning message.
So I don't see how metadata would be more or less scalable than a different status message.
Even if we end up with 20 different statuses, the big part is the switch of conditions.
orderNumber: 'MUI-123', | ||
scope: 'pro', | ||
licensingModel: 'subscription', | ||
planVersion: 'Q3-2024', |
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 used to:
planVersion: 'Q3-2024', | |
planVersion: '2024-Q3', |
I wonder if we should change this. But I guess it's up to the license key generator logic.
@@ -1,3 +1,20 @@ | |||
export const LICENSE_SCOPES = ['pro', 'premium'] as const; | |||
export const PRODUCT_SCOPES = ['data-grid', 'date-pickers', 'charts', 'tree-view'] as const; | |||
export const PLAN_VERSIONS = ['initial', 'Q3-2024'] as const; |
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 not convinced about "initial". To me, what we have today is
export const PLAN_VERSIONS = ['initial', 'Q3-2024'] as const; | |
export const PLAN_VERSIONS = ['Q3-2022', 'Q3-2024'] as const; |
the last time we changed the plan.
@@ -14,9 +15,9 @@ const oneDayInMS = 1000 * 60 * 60 * 24; | |||
const releaseDate = new Date(3000, 0, 0, 0, 0, 0, 0); | |||
const RELEASE_INFO = generateReleaseInfo(releaseDate); | |||
|
|||
function TestComponent() { | |||
const licesenStatus = useLicenseVerifier('x-date-pickers-pro', RELEASE_INFO); |
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.
Oops, I wrote licesen 🤪
export type ProductScope = (typeof PRODUCT_SCOPES)[number]; | ||
export type PlanVersion = (typeof PLAN_VERSIONS)[number]; | ||
|
||
export const extractProductScope = (packageName: string): ProductScope => { |
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.
A small detail, default function convention we use is function
for top level scope and arrow function for lower scope; applied here would be:
export const extractProductScope = (packageName: string): ProductScope => { | |
export function extractProductScope(packageName: string): ProductScope { |
Signed-off-by: Michel Engelen <[email protected]> Co-authored-by: Andrew Cherniavskii <[email protected]> Co-authored-by: Flavien DELANGLE <[email protected]>
Changes to support: https://www.notion.so/mui-org/store-Weekly-meeting-2024-06-03-5a0ca5ee914c406eac43673b89b233a4?pvs=4#be21aefcaef24c7880610b7a7bcf49a8