feat: pass some chart options to DateAdapter

[dangreen]

#7859

[simonbrunel]

@kurkle it has been a very long time I didn't look at Chart.js, so not sure how my review will be valuable 😄

I'm not sure I like the suggested way to pass down "some" of the chart options to the adapters. I would probably make no assumption of what adapters need from the chart options and pass the entire chart option hierarchy. Adapters can them fallback their options on whatever other chart options they want. This is actually similar to the scale init() method.

Also, I'm not a fan of storing this chartOptions as part of the DateAdapter. If adapters can override the constructor, I would just pass chartOptions as second argument, else have an init() method that takes this chartOptions as unique argument, so adapters can initialize their options with defaults.

_adapters._date.override({
  _id: 'luxon',

  // Not sure that override works!
  constructor(options: LuxonAdapterOptions, { locale }: ChartOptions) {
    this.options = { locale, ...options };
  }
})

// else

_adapters._date.override({
  _id: 'luxon',

  // New adapter API
  init({ locale }: ChartOptions) {
    if (!this.options.locale) {
       this.options.locale = locale;
    }
  }
})

Thoughts?

[stockiNail]

@simonbrunel I agree with you and I think the init() methods could be the better solution (but that's my personal opinion). But the default init() shouldn't throw any exception (like all the others) but it should be empty.

[stockiNail]

@dangreen thank you very much because you anticipated me! I was waiting for version 4.0 (as tagged in the issue) ! 👍

[simonbrunel]

@stockiNail Agreed, init() seems better because adapters can't call the base constructor with super(). I also agree that init() should not be abstract and the base implementation should be empty.

[kurkle]

@simonbrunel I just knew you'd have some good points, especially when talking interfaces, thanks!

init sounds good to me, with a default implementation. My initial thought was to resolve the locale to the adapter options, but as the values are incompatible between adapters, that would not work well.

I'd tend pass just the locale and maybe core version.
I can't think of anything else in the chart options that should affect date adapter behavior.

Edit: to be clear, I'm not against passing the whole chart options.

[dangreen]

Changed it to init method.

[kurkle]

Lgtm

[dangreen]

@kurkle @LeeLenaleee Can we move it to v3.9? As is does not contain breaking changes.

[simonbrunel]

@kurkle I'd tend pass just the locale and maybe core version.

That was a suggestion. I'm also fine to pass only { locale } to the init() method since locale is at the root of the chart options. If later there is a need for more options, we can pass the entire chart options which would simply extends { locale }, so no breaking change. However, I would not pass an object which has a different structure than the chart options and would cherry pick a few (potentially flatten) options based on use cases.

Whatever you guys think is better!

[LeeLenaleee]

@kurkle @LeeLenaleee Can we move it to v3.9? As is does not contain breaking changes.

It was already on 3.9.0 milestone :)

[dangreen]

@LeeLenaleee issue was in v4

[etimberg]

Just catching up on the discussion. Looks good to me 👍
I like the idea of passing all the chart options since it hopefully removes the need for future work later