Check colors definition in the chart options

[stockiNail]

Fix #11002

[stockiNail]

@dangreen Thank you for review! Applied and a bit changed (options is the reference to plugin options, provided to the hook).

[kurkle]

There is still room for improvement after this.

This could be checked by iterating the chart.configdatasetElementScopeKeys(type, elType)

But that would still leave out the defaults.datasets and defaults.elements etc.

Yet another option could be to use the dataset options resolver, but also check if the value is the value of defaults.color
and consider it undefined in that case too.

IMO best option overall would be to improve the options resolver to be able to tell you where the option is resolved from.

[DavideViolante]

It seems fine to me, but 2 functions with almost same name is a bit odd: containsColorsDefinitions, containsColorsDefinition.
If one is checking datasets and the other one options, I would call them containsColorsDefinitionsInDatasets, containsColorsDefinitionsInOptions. Or make one function for both.

[stockiNail]

IMO best option overall would be to improve the options resolver to be able to tell you where the option is resolved from.

@kurkle Can I try to implement it or do you want to do it?

[kurkle]

It can be done later, if ever. This should be ok for most users already.

[stockiNail]

yes, not for this PR. I'm curious to understand how to expose a method (or whatever else) to a Proxy in order to get the scope where the prop is resolved.

[stockiNail]

@kurkle just FYI (just a prototype):

Added a property to the cache of the proxy

    _search: () => function(key) {
      for (const scope of scopes) {
        if (!scope) {
          continue;
        }
        const value = scope[key];
        if (defined(value)) {
          return scope;
        }
      }
    },

  chart.options._search('backgroundColor'); // ==> returns the scope, for instance Chart.defaults.

[kurkle]

Looks simple enough 😃
Could also refactor the scopes to contain a name..

[stockiNail]

Could also refactor the scopes to contain a name..

This is more complex.... The "name" should be stored when a scope is calculated but in a different object (I don't think we could add a property to the scope or maybe yes with a special value, i.e. "_name"). We could also "wrap" the scope in an object, with a name property, but this could be quite disruptive, where the scopes are created and used. Let me spend some days to have a look.

[kurkle]

I think it could be a WeakMap usink the scope object as key and name as value.

[stockiNail]

I think it could be a WeakMap usink the scope object as key and name as value.

yes, even if I need to understand how to relate the key (scope) to the chart in order to keep the map "updated" otherwise I see the risk that it can grow maintaining useless reference. But not sure about that.

EDIT: That is, an object's presence as a key in a WeakMap does not prevent the object from being garbage collected. Once an object used as a key has been collected, its corresponding values in any WeakMap become candidates for garbage collection as well — as long as they aren't strongly referred to elsewhere.