Skip to content

Fix Chart.js v3 compatibility #30

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix Chart.js v3 compatibility #30

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/plugins/plugin.colorschemes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,8 @@ var ColorSchemesPlugin = {
}
};

Chart.plugins.register(ColorSchemesPlugin);
// Chart.js v3 uses "Chart.register", while v2 uses "Chart.plugins.register"
const registerPlugin = Chart.register || Chart.plugins.register;
registerPlugin(ColorSchemesPlugin);

Comment on lines +184 to 187
Copy link

@benmccann benmccann Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better solution would be to remove the automatic registration or to do it only for v2 users

Suggested change
// Chart.js v3 uses "Chart.register", while v2 uses "Chart.plugins.register"
const registerPlugin = Chart.register || Chart.plugins.register;
registerPlugin(ColorSchemesPlugin);

Other plugins no longer do automatic registration because it registers the plugin globally. It's preferred to be able to register it on a per-chart basis. For example, see https://chartjs-plugin-datalabels.netlify.app/guide/getting-started.html#registration

Copy link
Author

@lars-sh lars-sh Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable from a developers perspective and I'd be fine with that, too. Though here are some reasons, why it might make sense to stay with the global registration:

  • Pattern of the Zoom Plugin
    I took the Zoom Plugin as it's quite a basic plugin and it's hosted inside the chartjs GitHub organization. They're still registering the plugin globally as of https://github.com/chartjs/chartjs-plugin-zoom/blob/master/src/index.js#L4 .
  • Simple Usage
    Similar to the Zoom Plugin this one is likely to be used by developers with less knowledge. Therefore it might make sense to simplify the getting started process.
  • Backwards Compatibility
    Keeping global registrations allows for in-place upgrades from Chart.js 2.

Copy link

@benmccann benmccann Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The zoom plugin has two entry points. The UMD entry point auto-registers. The ESM one does not: https://github.com/chartjs/chartjs-plugin-zoom/blob/master/src/index.esm.js

If we want to follow that plugin, then we should have two entry points here as well rather than forcing all users to auto-register

export default ColorSchemesPlugin;