fix: add data types passed to constructor to instance properties

[dangreen]

Fixes #11064

[LeeLenaleee]

Should also add a test that you can retrieve the properties you expect, since that is what broke now

[dangreen]

@LeeLenaleee done

[LeeLenaleee]

This is incorrect, this should not return the bubble datapoint

[dangreen]

@LeeLenaleee
Is correct because:

// type T = number | [number, number] | Point | BubbleDataPoint
type T = ChartData['datasets'][number]['data'][number];

Plus, in a runtime data can be changed in any way

[LeeLenaleee]

No because you specify it as a line and that does not have the r prop so it shouldn't suggest it with ts. Also it should not rely on the type being changed at runtime

[dangreen]

@LeeLenaleee anyway, it is a problem with ChartData, not with changes from #10963, and it should be fixed in a separate PR.

[kurkle]

ChartData was changed in that pr, so I'm not sure how its not related. Afaik we can not achieve same level of detail in the types without templating.

[LeeLenaleee]

This did work before #10963 so it should either be fixed in this PR or #10963 should be totally reverted.

As you can see in this codesandbox

On line 31 I try to read the r prop and it does not let me because it does not exist

[dangreen]

@kurkle

ChartData was changed in that pr

ChartData interface and other interfaces that are used in ChartData were not changed in that PR

@LeeLenaleee

ChartData is not ChartData<"line", number[], string>. It worked because the data property in that chart.js version is locking for input data types, so in that case datasets with other chart type can't be added, and all tests from this file are failing: https://github.com/TrigenSoftware/Chart.js/blob/97772186316a70add5d5b262489b6409913bf4e3/types/tests/config_types.ts

[LeeLenaleee]

In my oppinion that would be better for now because having the current that you can access bubble datapoint on a line dataset by default is worse in my oppinion

[dangreen]

@LeeLenaleee

Above, we don't know the dataset type, so the datapoint can be number | Point | [number, number] | BubbleDataPoint | { category: string, value: number }. Here we know the dataset type, so here datapoint type is number | Point | { category: string, value: number }.

[dangreen]

@kurkle @LeeLenaleee @etimberg Review please 🙏

[LeeLenaleee]

Will take a look as soon as I am home again where I have access to my normal PC, should be either tonight or monday evening

[LeeLenaleee]

Old way:

This MR:

This should still not be the way, because it still sugest a bubble data point, it suggest a floating bar which are all wrong

[dangreen]

@LeeLenaleee It is because now you can change chart type on the fly without type errors.

The old way was that myChart.data is ChartData<'line'>, and it limits data mutation to another chart type.
In this PR myChart.data is ChartData<ChartType>, so now you can push new datasets with different types, and you can set new chart type without type errors.

You can't change generics on the fly. For example:

myChart = new Chart('ctx', { type: 'line' }) // Chart<'line'>

In old way here will type error:

myChart.type = 'bar'

And it can't change generic type to Chart<'bar'>. That's how TypeScript works. So we should make data property (and some others too) universal.

Anyway, in real world it is not a problem, because to do something with union type you should check it before:

var dpPoint = myChart.data.datasets[0].data[0]

dpPoint.r // type error, cause `r` doesn't exist in `number | [number, number] | ...`

if ('r' in dpPoint) {
  dpPoint.r // no errors, if it is not a BubbleDataPoint then this code just never runs
}

Even if chart type is line dpPoint should have number | Point type

1. in old way it is number and it also incorrect behavior, because it just gets type from constructor
2. number | Point it is a union, and again you should check type of the variable first

[LeeLenaleee]

And I still don't agree with that, if my editor shows me the type could be of type BubblePoint I excpect it to be possible of it being that type. Also I should not need to check if it it available each time in my eyes.

Still I stand by that I rather have the old behaviour as this new one where you can change the type.

So unless @etimberg and @kurkle are fine with this I am against it

[dangreen]

@LeeLenaleee What are we able to do with JS, we should be able to do with TypeScript without type errors. In the old way, it is not like that.

[kurkle]

@LeeLenaleee What are we able to do with JS, we should be able to do with TypeScript without type errors. In the old way, it is not like that.

You can, just add @ts-ignore. :)
But for real, I disagrer in that js allows using amything anywhere and that is precicely not what typescript is for.

[dangreen]

@LeeLenaleee @kurkle Ok, just for clear

const chart4 = new Chart('chart', {
  data: {
    labels: ['1', '2', '3'],
    datasets: [{
      type: 'bar',
      data: [1, 2, 3]
    }]
  }
});

chart4.data.datasets.push({
  type: 'line',
  data: [{ x: 1, y: 2}]
});

Do you think this example should work without type errors?

[kurkle]

Would it be too much to ask for "new Chart<'line'|'bar'>(...)" in that case?

[LeeLenaleee]

If I have to choose between that example and chart.js infering the correctly used types used at the time of chart initilisation I would choose the correct data types for your data array that are infered from initialization. But I am repeating myself so unless there is a way for both ways to co-exist I am against this and it should get reverted to what it was in my eyes

[etimberg]

Perhaps this is an indication that a future chart.js version should require the data-type for each dataset and dispense with the overall chart type.