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.

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

[v3-beta][types] fix a wrong Option infer #7770

Merged
merged 1 commit into from Sep 11, 2020
Merged

[v3-beta][types] fix a wrong Option infer #7770

merged 1 commit into from Sep 11, 2020

Conversation

xr0master
Copy link
Contributor

@xr0master xr0master commented Sep 7, 2020

The issue is that "infer O" takes type from the Dataset type instead of Options.

export type ConfigurationOptions<O> = O extends IChartConfiguration<unknown, unknown, unknown, infer O> ? O : never;

It should be:

export type ConfigurationOptions<O> = O extends IChartConfiguration<unknown, unknown, unknown, unknown, infer O> ? O : never;

However we cannot set the unknown type for the dataset because unknown cannot be IChartDataset. Then I thought, why do we need to use an unknown type for no-need types? We can use the type definition everywhere and only return the type we need. Let me know what do you think about it.

@xr0master
Copy link
Contributor Author

Rebased. I'll explain a little later why there is a bug.

@xr0master
Copy link
Contributor Author

When we want to overwrite options of chartjs, we can see this issue: Property 'data' is missing
image
However, we don't have any "data" in the options. After checking the source, we can see that in the ConfigurationOptions "infer O" points on the dataset type, we can also see this in the type:
Screen Shot 2020-09-09 at 6 30 36 PM
So it is.

@etimberg etimberg requested a review from kurkle September 9, 2020 20:52
@etimberg
Copy link
Member

etimberg commented Sep 9, 2020

I think I follow what this is doing. @sgratzl thoughts as well?

@kurkle
Copy link
Member

kurkle commented Sep 10, 2020

Only possible issue that comes to mind, is mixed charts:

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

Not quite sure if it is a problem, but I hope @xr0master can spot the issue if it exists :)

@sgratzl
Copy link
Contributor

sgratzl commented Sep 10, 2020

looks good to me

@xr0master
Copy link
Contributor Author

@kurkle in very short, the issue is about the wrong position.
the IChartConfiguration type has 5 "arguments" and 5th is Options, however infer O points on 4th "argument". So, I moved the infer O to the 5th position. That's all this fix does.

@xr0master
Copy link
Contributor Author

Only possible issue that comes to mind, is mixed charts:

I just now realized what the issue is in this example.
If I understand correctly, the purpose of these I*ControllerConfiguration types were to solve this problem. However, they are not used...

@xr0master
Copy link
Contributor Author

xr0master commented Sep 11, 2020

So, if we want to strict types, we can do it like this:

const barChart = new Chart<number, string, IBarControllerConfiguration>(node, {
  type: 'bar',
  data: {
    datasets: [{
      type: 'bar',
      data: [],
    }]
  },
  options,
  plugins,
});

BTW dataset do not have type property, TS fails.
image

@emmcbd FYI

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

I think this one does not really have anything to do with the mixed chart issue, so that can be solved separately.

@xr0master
Copy link
Contributor Author

xr0master commented Sep 11, 2020

Yes, of course, I just noticed. To be honest, I'm not sure if there is an issue at all with mixed charts.

@etimberg etimberg merged commit 9b00ff3 into chartjs:master Sep 11, 2020
@etimberg etimberg added this to the Version 3.0 milestone Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants