-
Notifications
You must be signed in to change notification settings - Fork 11.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add data types passed to constructor to instance properties #11071
fix: add data types passed to constructor to instance properties #11071
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also add a test that you can retrieve the properties you expect, since that is what broke now
@LeeLenaleee done |
}] | ||
}); | ||
|
||
// number | Point | [number, number] | BubbleDataPoint | { category: string, value: number } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect, this should not return the bubble datapoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LeeLenaleee anyway, it is a problem with ChartData
, not with changes from #10963, and it should be fixed in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ChartData was changed in that pr
ChartData
interface and other interfaces that are used in ChartData
were not changed in that PR
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
if (dataset.type === 'line') { | ||
// number | Point | { category: string, value: number } | ||
const lineDatapoint = dataset.data[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 }
.
@kurkle @LeeLenaleee @etimberg Review please 🙏 |
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 It is because now you can change chart type on the fly without type errors. The old way was that You can't change generics on the fly. For example:
In old way here will type error:
And it can't change generic type to Anyway, in real world it is not a problem, because to do something with union type you should check it before:
Even if chart type is line
|
And I still don't agree with that, if my editor shows me the type could be of type 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 |
@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. :) |
@LeeLenaleee @kurkle Ok, just for clear
Do you think this example should work without type errors? |
Would it be too much to ask for "new Chart<'line'|'bar'>(...)" in that case? |
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 |
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. |
Fixes #11064