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 compile errors when using multiple plugins #376

Closed
wants to merge 1 commit into from
Closed

Fix compile errors when using multiple plugins #376

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
2 changes: 1 addition & 1 deletion types/index.d.ts
View file
Open in desktop
Expand Up @@ -10,7 +10,7 @@ declare module 'chart.js' {
datalabels?: Options;
}

interface PluginOptionsByType<TType extends ChartType> {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand that change: this declaration is identical to the one in the core library. Maybe you can explain why it's required to add a default generic value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonbrunel first of all, let me say I don't have much experience on TS therefore I cannot explain you well why that's needed.
We had the same issue on annotation and zoom and I raise the issue on Slack Chart.js dev channel. @SebastiaanSafeguard had a look and he found this solution (that's working).
The bug seems to be introduced since Chartjs 4.1.0 by PR chartjs/Chart.js#10963.

It seems it needs to have the same type for interfaces merging. It wasn't possible to change only Chart.js without breaking other stuff.
I know that's not a good explanation.

Copy link

Choose a reason for hiding this comment

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

I am not entirely sure why this would be a breaking change exactly? I believe the only thing this does is help out the typescript compiler to see that multiple plugins are in fact using the same types.

Changing the declaration in the core library does not work, most likely because TSC needs to merge the different declarations and it is starting with core as ground truth.

What I believe is happening is that it tries to merge the declarations, and it has 3 (or more) type variables: TType, TType and TType. It only manages to merge two of the three(+). Even though they are meant to all be the same. As given by the error:

Error: node_modules/chart.js/dist/types/index.d.ts:2804:12 - error TS2314: Generic type 'PluginOptionsByType<TType, TType>' requires 2 type argument(s).

2804   plugins: PluginOptionsByType<TType>;

It thinks that PluginOptionsByType<TType, TType> is a thing here, even though there is only a single generic type.

Adding a default type seems to help the compiler to figure out that they can actually all be the same (since the default type can be a type for all type variables) thus allowing that the declarations are merged.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your replies.

I am not entirely sure why this would be a breaking change exactly

Because we are changing the declaration merging and this may impact people who have a working integration.

Is there an easy way to reproduce this issue? Can someone share a repo with minimal setup that shows this error?

Copy link
Member

Choose a reason for hiding this comment

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

@stockiNail The bug seems to be introduced since Chartjs 4.1.0 by PR chartjs/Chart.js#10963.

Maybe it would be better to investigate if that PR didn't introduce wrong code instead of spreading the default generic in all plugins (and beyond). If it's indeed the cause, maybe @dangreen could try to explain why his change have so much impact and maybe figure out if there is a better way to fix the issue mentioned in chartjs/Chart.js#10963.

To me, the current PR seems to introduce a bad workaround that I'm worry will backfire later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there an easy way to reproduce this issue? Can someone share a repo with minimal setup that shows this error?

chartjs/chartjs-plugin-annotation#854 (comment)

In the issue there is a zip file with a project where you can reproduce the issue with zoom and annotation plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe @dangreen could try to explain why his change have so much impact and maybe figure out if there is a better way to fix the issue mentioned in chartjs/Chart.js#10963.

@dangreen this is the issue that I raised some weeks ago in Slack and you had a look. New info is that the bug seems to be introduced by chartjs/Chart.js#10963, Chart.js version 4.1.0.

To me, the current PR seems to introduce a bad workaround that I'm worry will backfire later.

@simonbrunel for me it's fine ;). I fully agree to find out the best solution even if my poor knowledge in TS cannot help me to contribute too much on that.

Furthermore the PR chartjs/Chart.js#11244 has been approved and merged which fixed the doc, about how to implement the extension adding the default, and therefore I thought this workaround sounded ok!

Copy link

Choose a reason for hiding this comment

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

Because we are changing the declaration merging and this may impact people who have a working integration.

No? The merging is not changing? Maybe the merged declaration would change, but that is also not the case because the type parameter TType is still the same thing.

Anyways. Based on some testing I am not sure chartjs/Chart.js#10963 is the culprit. Reverting that in a local install does not fix the problem. There seems to be a difference in the way the types are packaged between 4.0.x and >4.1.x though (the latter has a full types folder in dist), which indicates to me that more changed. But I cannot find when or why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonbrunel @SebastiaanSafeguard FYI I have submitted an issue in Chart.js repo because this update is creating another issue. chartjs/Chart.js#11288 therefore I would agree with @simonbrunel that something is wrong on TS definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonbrunel @SebastiaanSafeguard I found why we have this issue. See chartjs/Chart.js#11288 (comment)

interface PluginOptionsByType<TType extends ChartType = ChartType> {
/**
* Per chart datalabels plugin options.
* @since 0.1.0
Expand Down