Convert axis options from arrays to objects

[benmccann]

This takes #6626 a bit further by updating all the fixtures. There's still a bunch of failing tests though

I won't have much time for the next couple weeks, so feel free to take this over. Hope this helps drive it forward a bit further since it's a big change

[etimberg]

Thanks for rebasing this @benmccann.

The biggest risk / flaw I have with my work in #6626 is deciding how to default axes for different charts. To me, having the user specify a 2nd Y axis by defining y1 on the scale object is indistinguishable from wanting to replace the default y axis with an axis that has an ID y1. At least in the old behaviour, one could change the default axis ID without worry.

Perhaps this means that arrays, while flawed, are the best way to handle it within the confines of config merge. The other possibility is that we need to go back to the drawing board and rethink the concept of merging options entirely. If charts did not specify axes in their defaults, this problem would be a non-issue.

[benmccann]

It's not totally clear to me how things work today. If I specified the below configuration it would end up with only one y scale?

    options: {
        scales: {
            yAxes: [{
                id: 'yAxisLeft',
                type: 'linear',
                position: 'left'
            }]
        }
    }

But you think there's some reason we couldn't have the code below result in the same outcome?

    options: {
        scales: {
            yAxisLeft: {
                type: 'linear',
                position: 'left'
            }
        }
    }

[kurkle]

I think its straight forward. If dataset has a scale ID define, update or create one with that ID. If not, use the first scale on that axis (maybe first with the default type? ) and apply defaults over undefined properties.

[benmccann]

If dataset has a scale ID define, update or create one with that ID.

I'm assuming you mean get or create one with that ID. It doesn't seem like you'd ever need to update a scale in this scenario. I would be okay with this behavior though I think it might be good to log a warning or even throw an exception instead since it seems like a user error to specify a scale ID that doesn't exist yet on a dataset.

If not, use the first scale on that axis (maybe first with the default type? ) and apply defaults over undefined properties.

Yeah, using the first scale makes sense to me

[etimberg]

It's not totally clear to me how things work today. If I specified the below configuration it would end up with only one y scale?

    options: {
        scales: {
            yAxes: [{
                id: 'yAxisLeft',
                type: 'linear',
                position: 'left'
            }]
        }
    }

But you think there's some reason we couldn't have the code below result in the same outcome?

    options: {
        scales: {
            yAxisLeft: {
                type: 'linear',
                position: 'left'
            }
        }
    }

That's exactly it.

I think its straight forward. If dataset has a scale ID define, update or create one with that ID. If not, use the first scale on that axis (maybe first with the default type? ) and apply defaults over undefined properties.

I'm not sure how the first scale is defined in this case because if we look at @benmccann's example above: after config merge there are two Y scales defined in the options: y and yAxisLeft. I have some reservations as to whether or not ignoring defined scales would be what the user wanted in that case.

[benmccann]

after config merge there are two Y scales defined in the options: y and yAxisLeft

I don't think that has to be the case. We could make it so that y is only created if the user does not specify any scales of their own just as is the case today

[kurkle]

Made a proposal for the merging in #6773
IMO it works more consistently than master, always applying the dataset provided 'axis' defaults, but not over anything user wrote in scales. So bar chart has offset: true, whether or not the scale is category (bunch of issues with time drawing half of the bar outside bounds, because the offset is not carried over)

[benmccann]

Closing in favor of #6773