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

[dperera-innerspec]

Expected behavior

Updating a patch version shouldn't cause types to start bleeding or crash. This has already happened before already. I get that this is to ease development, but having 2 different types for a value doesn't seem like a good practice.

Current behavior

It seems the new types for many structures like legend and tooltips have been allowed to be either a boolean or a structure, so when updating legend.display or tooltips.enable the types will crash.

Reproducible sample

https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAbzgYQBYENZwL5wGZQQhwDkAxhrAHQBWAziQNwCwAUGwG6ZxkwAecALxwAJhDIBXEAFMAdjCoBzaTACiAG2kz5AIQCeASREAKAEQg9aTDFMBKOOjpwAEgBUAsgBlk6WVzoaWnIwLKxcUHAWVljCstIA7iiUMMa8fAA0iGxwcDB6YNIAXKTqwHEk6dmi6DDoxQhVOeroAEbS6nTFANqmAErSIqaZpjrqEtJDcKYAmu3qEPGTpgDiUNJySwAKElBgmksA8lC+yqYAupWsOTkiNY4qnXBdDVfXTa3txSQAxHAQeHAAGoQGDSBiXN43O7dACMACZMjCAJyZADMmQArJkEXBURdGjgzlVsBC-mAYMAILJHi83nsJIoyjSCTkYBAIOoKWB6izrnJWpoRMU8OgOtJedgCSSCXQyKKwTzXm8+IqJaS3npVUrrpLtbqcrrsLZQmxgADjFFklQIOTKdSqPTGdSAPxUNkcrlwACEsQk6nUtiqlus1ttVLoDrGToj7s5wDAVH5LUFQlyUHGjCAA

Optional extra steps/info to reproduce

No response

Possible solution

Either keep the old structure instead of a boolean / structure format; or add a property "enable" that will act as the new boolean so when false the rest of properties do not need to be set.

Context

No response

chart.js version

v4.3.1

Browser name and version

No response

Link to your project

No response

[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)

[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) ?

[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

[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

[maxhellwig]

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

[LeeLenaleee]

4.3.2 has been released