Incorrect context for plugin options when scriptable

[stockiNail]

Expected Behavior

It's a special case but could happen.
When you are accessing to a plugin (i.e. datalabels) option which has been defined as a callback, by chart.options, for instance clicking a button in the DOM, the expectation is that callback is invoked with the right context in order to have the correct value.

var myChart = new Chart(ctx, {
    type: 'bar',
    plugins: [ChartDataLabels],
    data: {
      labels: ["Jan", "Feb", "Mar", "Apr", "May"],
      datasets: [{
         label: 'dataset',
         backgroundColor: "red",
         borderColor: "red",
         data: [120, 23, 24, 45, 51]
        }]
    },
    options: {
      responsive: true,
      plugins: {
         legend: true,
         title: true,
         datalabels: {
           color: 'white',
           display(context) {
             console.log(context, context.datasetIndex);
             return true;
           }
         }
      }
    }
});
document.getElementById('randomize').addEventListener('click', function() {
  console.log(myChart.options.plugins.datalabels.display); 
});

Current Behavior

With above sample, the context passed to display callback when you click on the button is not the correct one.
If the callback is implemented to access to context properties (in this case, being datalabels, datasetIndex or dataIndex) the value are not there and then you will get an error.
The context passed is the chart context.

Here log click randomize (see codepen below).

Steps to Reproduce

https://codepen.io/stockinail/pen/XWXgrdO

See codepen sample log.

Environment

• Chart.js version: [email protected], [email protected]
• Browser name and version: FF 86.0

[kurkle]

I think its the correct behavior, the option is not resolved in plugin context.

Either way, it can't be fixed in current design, because its the plugin that sets the context when it accesses the options.

[stockiNail]

I was thinking it's working as designed.
But it could happen anyway and therefore who will develop a scriptable options should take care about, checking the consistency of the context because it could be called with an unexpected context.
Do you think this could be a guideline to share?

[kurkle]

In v2, you'd get the function in that case. Might be worth documenting.

[stockiNail]

In v2, you'd get the function in that case. Might be worth documenting.

I fully agree!

[simonbrunel]

console.log(myChart.options.plugins.datalabels.display);

That's one of the reasons proxied options should not be exposed publicly. It magically (and wrongly) hides the context logic to the user who should actually expect the same result as v2. The datalabels options are guaranteed to receive a datalabels context and trying to access these options manually should force the consumer (here @stockiNail) to pass the correct context using helpers.resolve().

[stockiNail]

@simonbrunel @kurkle FYI, I have found a workaround for my lib in order to the callback is not invoked if the context is not what I'm expecting, therefore I'm not in hurry to have this behavior fixed.

My personal opinion, if I may.
I think the proxy implementation looks like cool because on almost all cases works very well! I have to say that I spent a lot of time to wrap it in the correct way. mainly because I'm not a JS expert.
But now, having understood how it works, it's really interesting and helpful. Unfortunately not with some cases like this one.
I don't want to force you to change anything because as I said it looks like a special case.
I don't know who will access to plugin options by chart.options without any awareness that property is set as scriptable.
Usually you are.

Maybe other Chart.js's wrapper libs (like mine) could face this issue, but I'm not sure.

[stockiNail]

Sorry if I'm annoying you... I have recognized that if inside the callback you will refer the same property by chart.options, you will entry in a never-end loop.
I know that's a stupid thing but in V2 (even if stupid) it worked without errors:

display(context) {
  const s = context.chart.options.plugins.datalabels.display;
  return true;
}

you get Uncaught Error: Recursion detected: display->display.
I have added this just for common awareness.

[kurkle]

@stockiNail that is correct. You can also get similar error with longer chains of options, if you did for example:

enabled(context, opts) {
  return opts.display;
},
display(context, opts) {
  return opts.enabled;
}

You'd get Uncaught Error: Recursion detected: display->enabled->display or Uncaught Error: Recursion detected: enabled->display->enabled depending on what property is accessed first.

[kurkle]

in V2 it worked without errors

I would not say it "worked", but you'd get no errors in that case. The s in your example would be reference to the same function you are currently in, so if you tried to resolve that, you'd be in recursive loop.

[simonbrunel]

Maybe other Chart.js's wrapper libs (like mine) could face this issue, but I'm not sure.

@stockiNail Actually, that's a good point. Proxied options are indeed nice but plugins and integrations should be able to access unresolved (i.e. raw) options as passed by the user without implicit fallback. I'm not sure that's still possible? Maybe via chart.config.options?

[kurkle]

@simonbrunel the chart.config.options is decorated on the scales part, to include all the relevant defaults. plugins key is also added, if missing. Ideally it would be the user provided, unchanged options.

[simonbrunel]

Agreed, ideally. But is chart.config.options proxied? Is it possible for a plugin / integration to access a scriptable option unresolved (i.e. as a function)? Or any option without implicit fallback (undefined option would return undefined)?

[stockiNail]

@simonbrunel as far as I know chart.config.options are not proxied. In my opinion you could use chart.config.options but, being created by the user to configure the chart, it couldn't contain all options but only what the user want to configure.
That means that a plugin or an integration should fallback the defaults (more or less merging as is in version 2 or implementing proxy logic by itself) because the user could have configured some defaults.
But maybe I'm wrong.

[kurkle]

chart.config.options is not proxied