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 behaviour should be Opt-in, not Opt-out #42

Closed
sect2k opened this issue Mar 22, 2018 · 22 comments
Closed

Plugin behaviour should be Opt-in, not Opt-out #42

sect2k opened this issue Mar 22, 2018 · 22 comments

Comments

@sect2k
Copy link

@sect2k sect2k commented Mar 22, 2018

First of all thanks for your work on this plugin, much appreciated.

Right now, when adding the script to page or importing it, it automatically add itself to all charts, which is quite a big assumption on how it's to be used and requires a lot of work to disable it on instance to instance basis.

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Mar 22, 2018

This has already been (more or less) discussed in #8 and #20. I think I agree that it should be disabled by default, however it's currently not that complicated to disable labels for all instances, it's one line:

// Globally disable datalabels
Chart.defaults.global.plugins.datalabels.display = false

This default behavior can't be changed until the next major release since it's a breaking change. Though, I'm not sure about changing it because it may generate more tickets from people reporting that the plugin doesn't work because they didn't set display: true.

@sect2k
Copy link
Author

@sect2k sect2k commented Mar 22, 2018

@simonbrunel thanks for the quick reply, I ended up doing something similar. I guess I should have searched better, didn't occur to me to check closed issues.

A possible solution would be to keep current behavior for the UMD build, but have it be opt-in when imported as a module. This way most people would not be affected by this change.

@jledentu
Copy link

@jledentu jledentu commented Jul 24, 2018

@simonbrunel Thanks for your tip ! Unfortunately, it doesn't disable the plugin, and a loop on all items of data is still executed. On charts with a lot of data, that causes performance issues.

I prefer to unregister the plugin globally, but I confirm that it would be great to not register the plugin by default. :)

Though, I'm not sure about changing it because it may generate more tickets from people reporting that the plugin doesn't work because they didn't set display: true.

A good MIGRATION tutorial, explaining that the plugin must be registered explicitly, could solve that, I think. But I may be too optimistic. 😃

@kumarharsh
Copy link
Contributor

@kumarharsh kumarharsh commented Nov 26, 2018

Though, I'm not sure about changing it because it may generate more tickets from people reporting that the plugin doesn't work because they didn't set display: true.

Given how the annotation plugin behaves, I think it's reasonable to expect that people would be able to grasp that an explicit export is required. I would expect plugins to not do more magic ;-)

Right now, I'm kinda stuck in between the two options - I am rendering a chart in two contexts, one where data labels are required and one where it's interactive. But solution no. 1 as described here where I have to globally disable the plugin and enable it per instance is not working for some reason (I must be doing something wrong, will comment here when I solve it), and solution no. 2 causes the plugin to still run over all the chart data before deciding that it shouldn't be displayed, which is kinda useless.

@kumarharsh
Copy link
Contributor

@kumarharsh kumarharsh commented Nov 26, 2018

Okay, I figured out what was going wrong for solution 1. I had to use the same instance of the plugin in my components as returned by Chart.plugins.getAll()! The way I had organized my react modules made me write this:

import { Chart } from 'react-chartjs-2';
import 'chartjs-plugin-datalabels';

const [dataLabelsPlugin] = Chart.plugins
  .getAll()
  .filter(p => p.id === 'datalabels');
Chart.plugins.unregister(dataLabelsPlugin);

/* @HACK: globally unregister the datalabels plugin because it can't be enabled
per instance currently. Remove this when library updates. */
export { default as annotationPlugin } from 'chartjs-plugin-annotation';
export { dataLabelsPlugin };

Just leaving this here if someone else stumbles upon this problem.

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Nov 26, 2018

Given how the annotation plugin behaves ...

What do you mean? It seems to registers itself automatically as well, the only difference is that it also exports itself, which makes easier to globally unregister it:

import annotation from 'chartjs-plugin-annotation';

Chart.plugins.unregister(annotation);

I agree that in case of module imports, it would be better if the plugin doesn't register itself automatically (that's why I kept this ticket open and will change the behavior in 1.x). But first we need to figure out what to do in case the plugin is imported via HTML script? How to get the plugin handle to be able to register it locally or globally?

<script src="path/to/chartjs-plugin-datalabels.js"></script>
<script>
    var plugin = // ????
</script>

@kumarharsh
Copy link
Contributor

@kumarharsh kumarharsh commented Nov 26, 2018

I think @sect2k's suggestion about retaining the auto-registration only for UMD would be a good solution. Using compiler flags like the ones used with webpack can be used to conditionally add some code for auto-registering for UMD modules.

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Nov 26, 2018

@kumarharsh Can you share references about how to implement your suggestion? (we use rollup)

I found that rollup defines a global variable with the default export, so we could simply remove the auto-registration for everyone and do the following when using <script>:

<script src="path/to/chartjs-plugin-datalabels.js"></script>
<script>
    // Window.ChartJsPluginDatalabels is exported by rollup 

    // register globally
    Chart.plugins.register(ChartJsPluginDatalabels);

    // register locally
    new Chart('id', {
        plugins: [ChartJsPluginDatalabels]
    });
</script>

However, when using <script>, I'm not sure users want that extra step in order to use this plugin, but I could be wrong, in which case it makes things easier :)

@kumarharsh
Copy link
Contributor

@kumarharsh kumarharsh commented Nov 26, 2018

I'll look into rollup and see how it can be done, probably over the weekend... In webpack, I use the webpack.DefinePlugin to do this.

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Nov 26, 2018

Thanks @kumarharsh (please share your code here instead of creating a PR since these changes will not happen before v1).

I would actually prefer the same behavior for everyone (no auto-register) but since I don't use Chart.js (and so plugins), I don't know if my previous suggestion is acceptable for someone importing via <script>. @sect2k @jledentu @kumarharsh what do you think?

@jledentu
Copy link

@jledentu jledentu commented Nov 27, 2018

@simonbrunel I agree with you that it's better to have the same behaviour for everyone.

Some popular libraries have a different behaviour between AMD/CommonJS imports and includes as script. In Vue for instance, some plugins like the popular vue-router is used automatically if it's available as global variable, whereas it must be used explicitly when imported as AMD/CommonJS module. I personnaly don't like this behaviour that can create confusion, and inconsistency in docs/examples.

So in my opinion, your suggestion is totally acceptable. 👍

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Dec 3, 2018

@jledentu Thanks!

For now, let's export the plugin but keep it auto-registered until next major version to not break existing projects. It's still an enhancement because it will be easier to globally unregister the plugin until v1. For the global name of the plugin loaded via script, I'm thinking of window.ChartDataLabels: any objection or better suggestion?

@kumarharsh
Copy link
Contributor

@kumarharsh kumarharsh commented Dec 3, 2018

I guess you can name it window.ChartjsDataLabels to scope it better. But apart from that, the suggestion seems solid. I personally would opt for the approach of keeping it explicit everywhere and adding a line in docs about usage.

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Dec 3, 2018

That was my first idea but since the Chart.js class is called Chart, I think it's more consistent to call it ChartDataLabels (like Vue and VueRouter, not VuejsRouter).

I personally would opt for the approach of keeping it explicit everywhere...

I agree, starting v1.0, this plugin will not be automatically registered whatever the import method.

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Dec 5, 2018

A first step implemented in a8084bf (default export but still auto-registered) and documentation updated about the current behavior until version 1.

@benmccann
Copy link

@benmccann benmccann commented Jan 24, 2019

According to semver.org we should be safe to make breaking changes now:

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

Whether we want to or not is another discussion

@benmccann
Copy link

@benmccann benmccann commented Jan 24, 2019

If we do not auto-register, is it possible to register a plugin only for a single chart? It seems we might need to add the functionality to the main Chart.js library or better document it if it already exists. It looks to me like the register function can only be called globally today: https://www.chartjs.org/docs/latest/developers/plugins.html

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Jan 25, 2019

If we do not auto-register, is it possible to register a plugin only for a single chart?

@benmccann please read the documentation (using plugins) and the link I posted in my previous comment.

Whether we want to or not is another discussion

This change is planned for v1.0, this plugin is widely used via CDN, breaking at a minor version is not an option (https://cdn.jsdelivr.net/npm/chartjs-plugin-datalabels@0 returns the latest version).

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Dec 2, 2020

@sect2k @jledentu @kumarharsh I'm looking to (finally) release v1 and I would like to check with you if removing the auto-registration entirely (<script> and import) is still the way to go. I think that a consistent behavior is better but some other official plugins are taking different paths. For example, the annotation registers the plugin in UMD builds but not in ESM, which seems even more confusing since UMD can also be used with require/import - or I misunderstood something, which may actually be the case :) Ideally, we should also have consistency in the way official plugins behave.

What would be the benefits of auto-registering the plugin in some cases but not in others?

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Dec 29, 2020

Done in 13daa47 and will be released in v1.0.0. Getting Started updated and added this breaking change to the migration guide.

@s4m0r4m4
Copy link

@s4m0r4m4 s4m0r4m4 commented Jan 6, 2021

Hi @simonbrunel, I just started using your plugin (0.7.0) and came across the auto-register behavior. Just one person's perspective here, but I do think the new direction you're going with in v1.0 makes more sense to me. Thanks again for an amazing plugin!

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Mar 13, 2021

Closing since it has been released in v1.0.0-beta.1 (Chart.js 2.x) and v2.0.0-beta.1 (Chart.js 3.x)

Any help testing these 2 beta versions is welcome :)

Thanks everyone for your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants