Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: add data types passed to constructor to instance properties #11071

Closed
wants to merge 1 commit into from
Closed

fix: add data types passed to constructor to instance properties #11071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 33 additions & 19 deletions src/types/index.d.ts
View file
Open in desktop
Expand Up @@ -491,7 +491,7 @@ export declare class Chart<
readonly id: string;
readonly canvas: HTMLCanvasElement;
readonly ctx: CanvasRenderingContext2D;
readonly config: ChartConfigurationInstance;
readonly config: ChartConfigurationInstance<TData, TLabel>;
readonly width: number;
readonly height: number;
readonly aspectRatio: number;
Expand All @@ -504,7 +504,7 @@ export declare class Chart<
readonly legend?: LegendElement; // Only available if legend plugin is registered and enabled
readonly tooltip?: TooltipModel; // Only available if tooltip plugin is registered and enabled

data: ChartData;
data: ChartData<ChartType, TData, TLabel>;
options: ChartOptions;

constructor(item: ChartItem, config: ChartConfiguration<TType, TData, TLabel> | ChartConfigurationCustomTypesPerDataset<TType, TData, TLabel>);
Expand Down Expand Up @@ -3605,29 +3605,39 @@ export type DefaultDataPoint<TType extends ChartType> = DistributiveArray<ChartT

export type ParsedDataType<TType extends ChartType = ChartType> = ChartTypeRegistry[TType]['parsedDataType'];

export interface ChartDatasetProperties<TType extends ChartType, TData> {
type?: TType;
data: TData;
}
export type ChartDatasetProperties<
TType extends ChartType = ChartType,
TData = DefaultDataPoint<TType>
> = {
[type in ChartType]: {
type?: type;
data: DefaultDataPoint<type> | TData;
}
}[TType]

export interface ChartDatasetPropertiesCustomTypesPerDataset<TType extends ChartType, TData> {
type: TType;
data: TData;
}
export type ChartDatasetPropertiesCustomTypesPerDataset<
TType extends ChartType = ChartType,
TData = DefaultDataPoint<TType>
> = {
[type in ChartType]: {
type: type;
data: DefaultDataPoint<type> | TData;
}
}[TType]

export type ChartDataset<
TType extends ChartType = ChartType,
TData = DefaultDataPoint<TType>
> = DeepPartial<
{ [key in ChartType]: { type: key } & ChartTypeRegistry[key]['datasetOptions'] }[TType]
> & ChartDatasetProperties<TType, TData>;
> = {
[type in ChartType]: DeepPartial<ChartTypeRegistry[type]['datasetOptions']> & ChartDatasetProperties<type, TData>
}[TType];

export type ChartDatasetCustomTypesPerDataset<
TType extends ChartType = ChartType,
TData = DefaultDataPoint<TType>
> = DeepPartial<
{ [key in ChartType]: { type: key } & ChartTypeRegistry[key]['datasetOptions'] }[TType]
> & ChartDatasetPropertiesCustomTypesPerDataset<TType, TData>;
> = {
[type in ChartType]: DeepPartial<ChartTypeRegistry[type]['datasetOptions']> & ChartDatasetPropertiesCustomTypesPerDataset<type, TData>
}[TType];

/**
* TData represents the data point type. If unspecified, a default is provided
Expand All @@ -3640,7 +3650,7 @@ export interface ChartData<
TLabel = unknown
> {
labels?: TLabel[];
datasets: ChartDataset<TType, TData>[];
datasets: (ChartDataset<TType, TData> | ChartDataset<ChartType, TData>)[];
}

export interface ChartDataCustomTypesPerDataset<
Expand All @@ -3649,7 +3659,7 @@ export interface ChartDataCustomTypesPerDataset<
TLabel = unknown
> {
labels?: TLabel[];
datasets: ChartDatasetCustomTypesPerDataset<TType, TData>[];
datasets: (ChartDatasetCustomTypesPerDataset<TType, TData> | ChartDatasetCustomTypesPerDataset<ChartType, TData>)[];
}

export interface ChartConfiguration<
Expand All @@ -3673,4 +3683,8 @@ export interface ChartConfigurationCustomTypesPerDataset<
plugins?: Plugin<TType>[];
}

export type ChartConfigurationInstance = ChartConfiguration | ChartConfigurationCustomTypesPerDataset & { type?: undefined }
export type ChartConfigurationInstance<
TData,
TLabel
> = ChartConfiguration<ChartType, TData, TLabel>
| ChartConfigurationCustomTypesPerDataset<ChartType, TData, TLabel> & { type?: undefined }
48 changes: 48 additions & 0 deletions test/types/config_types.ts
View file
Open in desktop
Expand Up @@ -63,3 +63,51 @@ chart4.data.datasets.push({
type: 'line',
data: [1, 2, 3]
});

const chart5 = new Chart('chart', {
data: {
labels: ['1', '2', '3'],
datasets: [{
type: 'line',
data: [{
category: '1',
value: 1
}]
}]
}
});

chart5.data.datasets.push({
type: 'line',
data: [{
category: '1',
value: 1
}]
});

chart5.config.data.datasets.push({
type: 'line',
data: [{
category: '2',
value: 2
}]
});

const dataset = chart5.data.datasets[0];
// number | Point | [number, number] | BubbleDataPoint | { category: string, value: number }
Copy link
Collaborator

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

Copy link
Collaborator Author

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

Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator

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

const datapoint = dataset.data[0];

if (typeof datapoint === 'object' && 'category' in datapoint) {
// datapoint: { category: string, value: number }
datapoint.value = 2;
}

if (dataset.type === 'line') {
// number | Point | { category: string, value: number }
const lineDatapoint = dataset.data[0];
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 }.


if (typeof lineDatapoint === 'object' && 'category' in lineDatapoint) {
// datapoint: { category: string, value: number }
lineDatapoint.value = 2;
}
}