Global datasets options thought

[stockiNail]

Documentation Is:

• Missing or needed
• Confusing
• Not Sure?

Current documentation

Having a look to the current documentation, the chain to set the datasets options is:

• per dataset: dataset.*
• per chart: options.datasets[type].*
• or globally: Chart.defaults.global.datasets[type].*

What's not clear

There is another default setting for chart type, where global chart settings are stored in Chart.defaults[type].

Does it mean that you can set the datasets options also at global chart level?
If yes, with Chart.defaults[type].dataset object?
If not, is this missing or not applicable?

In my personal (and not relevant) opinion, the datasets object related to chart type should be defined into global chart options (Chart.defaults[type].dataset) and not into default options (Chart.defaults.global.datasets[type]) because you have already options for chart type.

Am I wrong?

About the chain, element objects are always overlap by datasets ones, therefore my understanding is datasets options are evaluated with the following priority:

• per dataset: dataset.*
• per chart: options.datasets[type].*
• per globally: Chart.defaults.global.datasets[type].*
• per chart: options.elements.*
• per global chart: Chart.defaults[type].elements.*
• per global options: Chart.defaults.global.elements.*

Correct?

[benmccann]

We're working on 3.0 right now, so also curious about how you would structure these if you didn't have to worry about backwards compatibility. E.g. we're considering removing the global from the namespace

Chart.js/src/core/core.defaults.js

Line 14 in 72df272

// TODO(v3): remove 'global' from namespace. all default are global and

[stockiNail]

@benmccann first of all let me say the current chain on options is really good and consistent, in my option, therefore we won't change it in the logic (maybe accepting to rename and move objects in the options tree, whatever it is).

Table with levels and objects path for dataset options:

level	instance	current path	proposed path
0	dataset	dataset.*	dataset.*
1	chart	options.datasets[type].*	options.dataset.*
2	default chart		Chart.defaults[type].dataset.*
3	default	Chart.defaults.global.datasets[type].*	Chart.defaults.options.dataset.*

Some thoughts about above table:

1. at level 1 (chart) the dataset options are managing more than 1object. I think because in mixed charts you can have datasets with different chart type (i.e. bar and line). Nevertheless it could be a special case and who want to configure the dataset with the different type can do in other levels
2. at level 2, I think we should add the dataset object into default chart option, to be aligned the chain of options.
3. at level 3, the dataset options for all chart types could be useless because you can configure them at previous level if there is a specific default to manage at chart level. Furthermore in this dataset node we can manage all common properties for all datasets types.

Going to version 3 (removing global), I have also change the path.

But now, there is another question: what about options.elements.*?
Currently this node is providing defaults (on all levels) to the datasets (afaik) even if, from developer point of view, I should know which elements are used for each dataset and it's not so user friendly.
Having added the datasets options, we could remove options.elements at all levels, moving all their properties into dataset object.

In this way it's clear that whatever default dataset options must be set into a dataset object and the chain is clear.

I know that could be complex and break backwards compatibility (at least the options.elements)...
And that's my personal view, and maybe it's not the best one.

[benmccann]

Yes, I agree we should remove the options.elements.*

Regarding Chart.defaults[type].dataset vs Chart.defaults.datasets[type] - One nice thing about the Chart.defaults.datasets[type] version is that the code to resolve the options can easily get the chart-level and dataset-level defaults separately and apply them in the correct order. Though I suppose it should be easy enough either way really

[stockiNail]

Regarding Chart.defaults[type].dataset vs Chart.defaults.datasets[type] - One nice thing about the Chart.defaults.datasets[type] version is that the code to resolve the options can easily get the chart-level and dataset-level defaults separately and apply them in the correct order

@benmccann I'm not 100% sure I understood correctly what you mean.

Let me try to explain what I thought.
The current way that Chart.js is using (and correct me if I'm wrong) if to merge all options in the order of level in order to have, when a chart is created, the options filled with all properties.

With the proposal nothing change because we can use the current way and every time you have got a dataset object with all properties which can be used to render the chart. And also the merging the dataset itself (passed by user) it's easy because it's enough to merge with chart options created as explained above.

Therefore I can not understand how the Chart.defaults.datasets[type] version could resolve the options easily.

Please forgive my lack of deep knowledge of internal implementation of Chart.js and take what I'm said using my previous assumption.

[stockiNail]

@benmccann Today I had a look to the master to check some changes of version 3.
I have the feeling that the datasets options will remain as is.
Maybe I'm wrong.
Nevertheless, in my opinion, to have datasets defaults for single type of it into common defaults is not correct because the common defaults should contain the defaults whatever is the type of element (chart, scale or dataset). In fact it works in this way for charts and scales.

[benmccann]

I wasn't necessarily against it, but no one had gotten to it. I just sent #6955. See if it matches your expectations

[stockiNail]

GREAT @benmccann !!

Really appreciated your support!

[etimberg]

I've merged #6955, is that enough to close this?

[benmccann]

@stockiNail please check the latest code from master when you get a chance and the accompanying migration guide and let us know if you have any other suggestions. I'm going to close this for now on the assumption that it's mostly addressed

[stockiNail]

@benmccann Perfect! I'm gonna update the project and have a look. I need a couple of days because busy in other stuff.

[stockiNail]

@benmccann finally I've checked right now! SOUNDS really good!
Thank you very much.
Having a look to changes from version 3.0, I'll work a lot to adapt my project, I guess !!

[benmccann]

Yes, a lot has changed. I hope the improvements make it worth it. Here's a migration guide to help your testing: https://github.com/chartjs/Chart.js/blob/master/docs/getting-started/v3-migration.md

[stockiNail]

I have already had a look...
To be honest there are a couple of changes that I don't like too much.

Chart.js is no longer providing the Chart.bundle.js and Chart.bundle.min.js.
moment is no longer specified as an npm dependency.
If you are using the time scale, you must include one of the available adapters and corresponding date library.
If you are using a date library other than moment, you no longer need to exclude moment from your build.

In my opinion, based on my experience, this kind of changes are moving effort to user side.
A chart library should always have time series, out of the box. In this way, you should get chart.js, the adapter and then date library, adding dependencies to check every time. Coming from Java world, I can say that the best libraries are the "without" dependencies ones.
I think Chart.js should provide a default bundle, with an adapter and date library as is today. Whoever needs to use a different date library, can change.

options.onClick is now limited to the chart area

I don't understand how reducing the scope on chart area can help. To have more listeners on the same dom element it does not make sense if you can have 1, as is today.

At the end of the day, I'll do my best to be 100% aligned to new version, just a matter of time.

[stockiNail]

Chart.js is no longer providing the Chart.bundle.js and Chart.bundle.min.js.
moment is no longer specified as an npm dependency.
If you are using the time scale, you must include one of the available adapters and corresponding date library.
If you are using a date library other than moment, you no longer need to exclude moment from your build.

In my opinion, based on my experience, this kind of changes are moving effort to user side.
A chart library should always have time series, out of the box. In this way, you should get chart.js, the adapter and then date library, adding dependencies to check every time. Coming from Java world, I can say that the best libraries are the "without" dependencies ones.
I think Chart.js should provide a default bundle, with an adapter and date library as is today. Whoever needs to use a different date library, can change.

@benmccann today I've released new vesion of my project and then I spent some minutes to go in deep about date adapter.

I thought moment will be out (adapter included) but going more in deep of the code, it sounds the default, as is today. Therefore what is not provided is ONLY the library itself and you don't have to include the adapter-moment. It's a little bit strange but better.
And futhermore, I have seen in details how the adapter interface is defined and maybe I'm gonna build something only for GWT (framework which I'm using).

[benmccann]

Work is still in progress on that piece. We've moved adapter-moment to a new repo. The plan was to remove it from the main repo after a release is cut on the new repo

One problem with creating a bundle would be which library do we include by default in the bundle? We have three different date adapters and everyone has their favorite one. Also, moment was used originally because it was one of the original date libraries, but it's much worse than the alternatives and I wouldn't recommend anyone to use it. It has a terrible API where everything is mutable and it's a huge library. You could use the Luxon adapter and get way more features (timezones, i18n, etc.) or the date-fns adapter and get a smaller library. Those would both be much better choices for any new project. But then there's a tricky thing of old users have moment, but we wouldn't want to promote it to new users. We'd almost have to create three bundles.

[stockiNail]

Got it!

Work is still in progress on that piece. We've moved adapter-moment to a new repo. The plan was to remove it from the main repo after a release is cut on the new repo

I saw it and also that it is not published in jsdelivr yet and I had the feeling that is still in progress.
I'm gonna think if to implement all available adapters (and user can decide what wants) or only one, decided by us. Luxor sounds better but I should check.

Thank you very much, @benmccann

[kurkle]

@stockiNail FYI this change was reverted by #8090. See #8073 for explanation.

[stockiNail]

@stockiNail FYI this change was reverted by #8090. See #8073 for explanation.

Thank you @kurkle
I have seen and not a big deal. I'm gonna change the code with new specification in next weeks, together with the other changes (like font color, tooltip/legend/title).
I want to wait for while because I:

1. need to work on documentation for new version
2. am working on Annotation plugin (some PRs could come)