-
Notifications
You must be signed in to change notification settings - Fork 478
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
Comments
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 |
@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. |
@simonbrunel Thanks for your tip ! Unfortunately, it doesn't disable the plugin, and a loop on all items of I prefer to
A good |
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. |
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
Just leaving this here if someone else stumbles upon this problem. |
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> |
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. |
@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 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 |
I'll look into rollup and see how it can be done, probably over the weekend... In webpack, I use the |
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 |
@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 So in my opinion, your suggestion is totally acceptable. 👍 |
@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 |
I guess you can name it |
That was my first idea but since the Chart.js class is called
I agree, starting v1.0, this plugin will not be automatically registered whatever the import method. |
A first step implemented in a8084bf (default export but still auto-registered) and documentation updated about the current behavior until version 1. |
According to semver.org we should be safe to make breaking changes now:
Whether we want to or not is another discussion |
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 |
@benmccann please read the documentation (using plugins) and the link I posted in my previous comment.
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). |
@sect2k @jledentu @kumarharsh I'm looking to (finally) release v1 and I would like to check with you if removing the auto-registration entirely ( What would be the benefits of auto-registering the plugin in some cases but not in others? |
Done in 13daa47 and will be released in v1.0.0. Getting Started updated and added this breaking change to the migration guide. |
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! |
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. |
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.
The text was updated successfully, but these errors were encountered: