-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Add missing feature for disabling plugins in TyeScript #11403
Conversation
This (renaming interfaces) can indeed possibly break some things. For now in v4 I think the best approach is to define the interfaces as this: export interface PluginOptionsByType<TType extends ChartType> {
decimation: DecimationOptions | false;
filler: FillerOptions | false;
legend: LegendOptions<TType> | false;
subtitle: TitleOptions | false;
title: TitleOptions | false;
tooltip: TooltipOptions<TType> | false;
}
export interface PluginChartOptions<TType extends ChartType> {
plugins: PluginOptionsByType<TType> ;
} This would make it fail to disable all plugins but I think that would be the use case that would be used the least and we never heared anymore complain about it. Worst case they can put a But with this we mostly solve the issue. Without too much breaking possibility. In theory this can still do some weird things but these are the more correct types. |
I agree with @LeeLenaleee. This could be a breaking change for other users of those types. |
@LeeLenaleee @etimberg thanks a lot for feedbacks! I have updated the PR as suggested by @LeeLenaleee and I also think it makes sense to do it int his way. |
|
||
Chart.defaults.scales.time.time.minUnit = 'day'; | ||
|
||
Chart.defaults.plugins.title.display = false; | ||
(Chart.defaults.plugins.title as TitleOptions).display = false; |
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 cast is not needed now anymore 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.
No, viceversa. It is now needed (or we need to use ts-ignore). With previous PR impl, having 2 interfaces and defaults was extending the interface without the possible false value) it wasn't needed. Now it's needed because TS can not recognize the type:
[types] test/types/defaults.ts(5,30): error TS2339: Property 'display' does not exist on type 'false | TitleOptions'.
[types] Property 'display' does not exist on type 'false'.
@@ -27,4 +27,4 @@ Chart.defaults.layout = { | |||
}, | |||
}; | |||
|
|||
Chart.defaults.plugins.tooltip.boxPadding = 3; | |||
(Chart.defaults.plugins.tooltip as TooltipOptions).boxPadding = 3; |
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.
idem
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.
See #11403 (comment)
I think this PR broke TypeScript code in 4.3.1 that has assignments like this: ChartJS.defaults.plugins.tooltip.bodyFont = {
size: 12,
} Error:
Workaround: if (ChartJS.defaults.plugins.tooltip) {
ChartJS.defaults.plugins.tooltip.bodyFont = {
size: 12,
}
} |
Fix #11398
@etimberg @LeeLenaleee @kurkle first of all and as you know, my experience on TS is limited. Therefore not sure if the issue can be solved differently.
Furthermore there are 2 thoughts:
DefaultsPluginOptionsByType
, see changes on the doc.