Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin options, as array, are passed as an empty object to plugin methods #7754

Closed
stockiNail opened this issue Sep 3, 2020 · 8 comments
Closed

Comments

@stockiNail
Copy link
Contributor

I'm testing some plugins, migrating to Chartjs 3.

I see a breaking change (I don't know if it is a bug) on plugin implementation between version 2.9.3 and 3.0.0.

Many plugin methods (like beforeUpdate) are receiving as argument the plugin options instance.

In the version 2.9.3, if the plugin is configured into the chart options by an array of objects, the array was passed as argument to the plugin methods.
Instead in version 3.0.0 if the plugin is configured into the chart options by an array of objects, a empty object is passed as argument to the plugin methods.

Here is codepen (with v3): https://codepen.io/stockinail/pen/dyMZoZX

Maybe it's not a bug but something that we want.

@kurkle
Copy link
Member

kurkle commented Sep 3, 2020

Do you know if there are many plugins expecting array as options?

@stockiNail
Copy link
Contributor Author

I don't know how many plugins are using an array as options.
In my lib, I have included 4 plugins (ootb) and only 1 is using an array.
Also having a look to other plugins, I haven't found another one which has got this kind of configuration.

The plugin is chartjs-plugin-labels. Honestly it does not seem so maintained.
For this reason I have forked more than one year ago and adapted for my purposes.

And I have already updated it in order to be used on ChartJs 3, therefore is not a showstopper for me.
The solution is ignoring the method parameter and use the options directly from chart instance.

I don't know if makes sense to change in order to be compliant with previous version. Personally I prefer to have an object to configure a plugin and not an array.
Anyway I wanted to notify it, maybe some one else will have the same issue, because you don't have any exception.

@kurkle
Copy link
Member

kurkle commented Sep 3, 2020

The options are merged with the defaults in v3, that's where it fails.
If we'd support that config, I think each element of the array needs to be merged with defaults.
@etimberg any thoughts?

@etimberg
Copy link
Member

etimberg commented Sep 3, 2020

Arrays would need to be merged per instance but it causes problems as we saw with the scale options. I'm happy to document it as a breaking change from v2. I suspect the unmaintained annotations plugin suffers from the same problem.

@stockiNail
Copy link
Contributor Author

Annotation plugin is configured in different way but it is using array to have multiple annotations therefore I suspect as well.
Also datalabels can accept an array of configuration objects.
In both cases the array is a subnode (not the main one as for labels plugin).
If the problem is whatever array of objects in the configuration and not only for the root node then could be an important breaking change.

@kurkle
Copy link
Member

kurkle commented Sep 4, 2020

Arrays are fine within the configuration object.

@stockiNail
Copy link
Contributor Author

Fine! That means only labels plugin has got this issue, not annotation or datalabels, therefore in my opinion I fully agree with @etimberg to leave as is today, documenting it.
I am gonna change my labels plugin code to be aligned.
Thanks a lot.

@stockiNail
Copy link
Contributor Author

Errata corrige: datalabels does not use array for multiple labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants