Adds type property to label context

[stockiNail]

As reported in CHART.JS issue chartjs/Chart.js#8623 and the related PR chartjs/Chart.js#8626, there is the following new guideline about context:

Note: the context argument should be validated in the scriptable function, because the function can be invoked in different contexts. The type field is a good candidate for this validation.

This PR is adding type property to label context.

[simonbrunel]

I don't think this type is useful to this plugin since it doesn't rely on the proxy logic to resolve the plugin options. That means it's guarantee that the plugin options are only called with the datalabels context but also that the plugin will never resolve core options with the plugin context.

But even if it was using the proxy logic, I don't fully get the benefit of this type in the context and I tend to think it's more confusing than useful, thus should be avoided. So if there is no good reason to have this type, I prefer to not include it in this plugin.

[stockiNail]

First of all, as I wrote in the chartjs/Chart.js#8623 issue in CHART.JS, this is a special case.

I don't think this type is useful to this plugin since it doesn't rely on the proxy logic to resolve the plugin options. That means it's guarantee that the plugin options are only called with the datalabels context but also that the plugin will never resolve core options with the plugin context.

As I described in the issue, above mentioned, accessing to chart.options.plugins.datalabels.display (because in codepen I used that property), the callback set for display property is invoked with chart context and not with context of the label.
And if I'm using the context thinking to have dataIndex or datasetIndex, I could get an error.

As @kurkle replied, this is the behavior of chart.options by design. That means the context is not definitely guarantee.

But even if it was using the proxy logic, I don't fully get the benefit of this type in the context and I tend to think it's more confusing than useful, thus should be avoided. So if there is no good reason to have this type, I prefer to not include it in this plugin.

In my opinion, I think it's helpful to have a property which describes the type of context (forgive me, I don't see any confusion but that could be my fault). The other way is to check if the context contains all needed properties (not undefined) and in many cases you should perform more than 1 if statement.
Furthermore, the PR chartjs/Chart.js#8626 has correctly (in my opinion) added in the documentation the following note:

Note: the context argument should be validated in the scriptable function, because the function can be invoked in different contexts. The type field is a good candidate for this validation.

Last but not least, the CHART.JS context contains the type property therefore the context of the plugin could keep aligned with the CHART.JS structure, when applicable.

[simonbrunel]

... the callback set for display property is invoked with chart context and not with context of the label.

As you said, it's a special case. The plugin options are intended to be consumed by the plugin itself. If you want to resolve these options outside of the plugin, it's your responsibility to provide a context compatible with the datalabels one or to provide a way to check the context for this special case.

As @kurkle replied, this is the behavior of chart.options by design. That means the context is not definitely guarantee.

Each option level must be guaranteed to receive a certain context shape:

• a root option should receive at least a chart context,
• a scale option should receive at least a scale context,
• a datalabels option should receive at least a datalabels context,
• etc.

You can't ask to check type === 'datalabels' when using a datalabels options ... and you shouldn't care about 'datalabels' when setting a core option.

I think it's helpful to have a property which describes the type of context...

I don't think it's a good approach to handle different contexts. You should never have to check type, it breaks abstraction of the core options by making them plugins (or features) aware:

• if you want to control options at the scale level, use the scale options,
• if you want to control options at the plugin level, use the plugin options,
• etc.

Don't make a root option aware of plugins, scales, etc.

... the CHART.JS context contains the type property ...

I'm probably missing some use cases that would make this type valid but I really feel it should not exist at all.

[kurkle]

@simonbrunel think about:

Chart.defaults.color = (ctx) => {...}

edit, or:

new Chart('id', {
  type: 'line',
  data: {
    labels: ['a', 'b', 'c'],
    datasets: [{
      label: 'test',
      data: [1, 2, 3],
      backgroundColor(ctx) => ctx.type ==='dataset' ? 'red' : 'blue' // latter is for points (etc, because not tested)
   }]
 }
}

[simonbrunel]

@kurkle I'm not sure I get your point, why would you ever need to write:

Chart.defaults.color = (ctx) => {
  return ctx.type === 'datalabels' ? 'red' : 'blue';
}

Instead of:

Chart.defaults.set({
  color: 'blue',
  plugins: {
    datalabels: {
      color: 'red';
    }
  }
}

About your second example, it could be:

backgroundColor: (ctx) => ctx.dataIndex === undefined ? 'red' : 'blue' 

The difference with scales, ticks and plugins is that there is no option level for 'data'. That should be the only case where you need to check the context shape before using it. And I don't think it justifies a type, especially to include scales, ticks, tooltip, and plugins: you should use the associated options instead.

[simonbrunel]

@stockiNail Thank you for your contribution, I really appreciate your effort in helping for the upcoming Chart.js v3 release.

I explained my concerns more in details in chartjs/Chart.js#8642 so it doesn't get lost. I'm closing this PR because I don't adhere to the use of type in the overall logic of scriptable options but if I missed a valid use case, I will be happy to re-evaluate this PR.

[stockiNail]

@simonbrunel ur welcome! nop for this PR. I don't want to push too much on this topic, being a special case.
I can understand also your point of view that's correct.
Anyway I have already found a workaround for my lib (good one, without type). :)