Moving from 4.3.0 to 4.3.1 break most of my already existing code due to types

[dperera-innerspec]

Moving from 4.3.0 to 4.3.1 break most of my already existing code due to types

[LeeLenaleee]

4.3.2 has been released

[Nico-DF]

Discussion linked to the commit/PR: #11403

Something like this might work as a "workaround":

if (myChart.options.plugins?.tooltip)
  (myChart.options.plugins.tooltip as TooltipOptions).enabled = true;

Nonetheless, I personnaly globaly agree with your remark (even though it was identified on PR).

Either we accept this change and we need to cast as specified in PR, or if we want something "smoother":
If I can make some suggestion too, either recheck all plugin types to have a enabled property (as @dperera-innerspec proposes) and return to old definition, or maybe use optional parameter to have some retro-compatibility (and yes, explicitly setting plugin to undefined is a bit ugly, but acceptable):

export interface PluginOptionsByType<TType extends ChartType> {
  colors?: ColorsPluginOptions;
  decimation?: DecimationOptions;
  filler?: FillerOptions;
  legend?: LegendOptions<TType>;
  subtitle?: TitleOptions;
  title?: TitleOptions;
  tooltip?: TooltipOptions<TType>;
}

And you disable by explicitly setting null/undefined?

PS: I'm no TS expert either, but I think this would be acceptable (but didn't dug into code to check if undefined would effectively disable plugin but in theory it should)

[dperera-innerspec]

@Nico-DF i think first option is better because declaring a structure as undefined means you don't need it while declaring as something means you want it to behave a certain way

[maxhellwig]

Hi, @stockiNail I can confirm, that #11422 solves the typing issues I had. Thank you very much!

[Nico-DF]

Another solution might be:

export interface ConfigurablePlugin {
  enabled: boolean
}

export interface PluginOptionsByType<TType extends ChartType> {
  colors: ColorsPluginOptions & ConfigurablePlugin;
  decimation: DecimationOptions & ConfigurablePlugin;
  filler: FillerOptions & ConfigurablePlugin;
  legend: LegendOptions<TType> & ConfigurablePlugin;
  subtitle: TitleOptions & ConfigurablePlugin;
  title: TitleOptions & ConfigurablePlugin;
  tooltip: TooltipOptions<TType> & ConfigurablePlugin;
}

I think it should also work (even if base type alread have enabled <- to confirm) ?

[LeeLenaleee]

The current typings were incorrect. We specifically chose this route since the other route would have introduced new interfaces and a change in interfaces which we forsaw being more breaking/problematic.

For now in my oppinion these typings correspond the best with how they can be used in the code. For V5 we can refactor them so setting the defaults would be possible again without casting

[dperera-innerspec]

@LeeLenaleee v4.3.1 typings do not allow me to change values unless i re-cast the whole structure whenever i want to access it. That surely is not easier than just allowing the interface to be undefined (already a falsy value) aside from the structure.

[Nico-DF]

The current typings were incorrect. We specifically chose this route since the other route would have introduced new interfaces and a change in interfaces which we forsaw being more breaking/problematic.

For now in my oppinion these typings correspond the best with how they can be used in the code. For V5 we can refactor them so setting the defaults would be possible again without casting

LGTM, I accept proper change being made only on v5, but I would recommand to explicitely add a small note/breaking note on current release with recommended changes (especially since it is a patch update)

Although the cast is not a huge change, I had to dig a bit to understand the source of change and why (and I think @dperera-innerspec was probably the same)

[LeeLenaleee]

@etimberg @stockiNail maby we should revert the change and keep it for V5 to fix.

I hear a lot more complains now then before that we could not pass false.

[stockiNail]

@LeeLenaleee I agree. Unfortunately I don't have much time because I'm deeply engage in new project. I'll do my best

[stockiNail]

@LeeLenaleee @etimberg PR to revert #11422