Deprecate configMerge and scaleMerge helpers

[simonbrunel]

These methods shouldn't have been public since they are specific to the chart controller internal logic. Note that this scale custom merging will be removed in v3.

Closes #5995 (private methods don't need jsdoc).

[benmccann]

I still think it may be helpful to add the jsdocs. I agree they're not required. I find them helpful sometimes as a newcomer to the codebase especially in an loosely typed language like ecmascript

[simonbrunel]

I removed the explicit clone of the first argument since it's already done by helper.merge() when passing an empty object as the target. It should be a bit more optimized and I feel it makes easier understanding the intent of these methods. Added an additional test to make sure we are not modifying the defaults when merging (the reason of the previous explicit clone).

@benmccann I also added short header comments which I hope help to understand why these methods. I would not spend more effort commenting this code since it should disappear in v3 and simply replaced by helper.merge().

[kurkle]

Couple of bytes can be saved, nothing else pops out.

[Lekishor]

Hi, quick question from a rather new user of chartjs.

Context:
I created some code around chartjs so that I could easily / quickly create any kind of graph with basic options already set for most of them.

In order to do so I currently use the "configMerge" helper so that I can merge a default conf object with a user defined object prior to creating the chart (using the merged result as its "options" param).

I understand why you say these are supposed to be private because they are "specific to the chart controller internal logic" but I thought it was a good thing they were exposed as helpers for cases like mine because I'm actually counting on the fact it has the same logic as the controller so that the conf passed to the chart constructor remain properly defined.

Question:
So, I was wondering if it had to really be private in the end? For me there is a purpose to keep those exposed. I still have a couple of releases to change things on my side if need be, it just made me think I might not be the only one using those helpers that way.

Thanks.

[benmccann]

@Lekishor can you just use helpers.merge for that?

[Lekishor]

@benmccann I could use helpers.merge sure, but as my user conf and default conf might contain options such as scale etc. it was "easier" to use the existing merge methods handling those options than re-writing something that's why for the time being I use configMerge

[simonbrunel]

I created some code around chartjs so that I could easily / quickly create any kind of graph with basic options already set for most of them.

I'm not sure why you need to rely on those methods, can share your code?

So, I was wondering if it had to really be private in the end.

These methods are still public but deprecated. They exist only because of a wrong design of the scale options and we don't want to provide support for using them externally. The deprecation is to discourage users to rely on those methods. In v3, scales options will be changed to an object instead of arrays and this logic will be completely removed.

We are not going to un-deprecate these 2 methods but you can safely continue to use them until v3.

[Lekishor]

@simonbrunel If scales is going to change and work as any other option in V3 then ok I won't need any other method that helpers.merge so that answer my question thanks.

About my code, I made a simple factory allowing me to create a chartjs graph and some surrounding HTML objects with a single call to a create method instead of rewriting everything every time.
I mostly need the same options for a given graph type so I predefined some default options objects and all I do is tell my factory create method what options I need (zoom, scales etc.) and from there it uses the objects I defined to create a proper config such as this :

{
      zoom:
      {
          enabled: true,
          drag: false,
          mode: 'x'
      },
      scales:
      {
            xAxes: [{
                  type: "time",
            }],
      },
}

But I also give the opportunity for a user to send its own options object to alter the default options, so a simple example would be :

{
    scales:
    {
        xAxes: [{
              stacked: true,
        }],
    }
}

In the end I just want a single objects with all my keys merged properly to send to the Chart constructor and that was what I used helpers.mergeConfig for as I was sure it would handle the merge properly for each existing options even scales.

[simonbrunel]

@Lekishor why don't you use Chart.defaults.* instead?

[Lekishor]

@simonbrunel Hum, to be honest I did not thought about using Chart.defaults.* for this.
In my mind this was to be modified once if you had a "fixed" predefined config you wanted to use every time you draw a specific type of graph and not something to change every time you create a new graph.

What I call my own default conf is something that is in fact modular and will change for almost each call made to my factory depending on the "options" requested as param (scales, zoom etc.) therefore it was more something to complete the user defined conf before creating the chart rather than being the actual setting of Chart.defaults. That way, the original Chart.defaults options were still there and merged with my own in the end. I wanted to merge things not replace the original default options so I coded this in order to alter the user conf passed to the graph instead.

Anyway, I think I'll go back to read the doc I might have missed something and give it a try at some point before the release of the V3 and see what happens.

Thank you for the answers / suggestions