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

Fix compile errors when using multiple plugins #376

Closed
wants to merge 1 commit into from

Conversation

stockiNail
Copy link
Contributor

Fix #374

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

I believe this is a breaking change for TypeScript users, so better to be sure why it's required considering that this interface is matching the one in the core lib.

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

@ghost ghost Apr 21, 2023

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)

@simonbrunel
Copy link
Member

Awesome findings @stockiNail! Thank you for your perseverance figuring out what was the source issue. Your conclusions make sense and it's good that you discovered that rollup config change so it does not impact other types in the future. I agree that it should be fixed in the core library - thus closing this PR.

@stockiNail stockiNail deleted the fixPluginType branch May 25, 2023 12:32
@stockiNail
Copy link
Contributor Author

Thank you too @simonbrunel . Unfortunately my knowledge on TS is not so good and I needed some days to discover it. Maybe I could find it before...

@simonbrunel
Copy link
Member

It was not an obvious issue and I'm sure you have learn a lot! Good job! 👍

@stockiNail
Copy link
Contributor Author

you have learn a lot!

Definitely YES! Thank you again @simonbrunel

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.

error TS2428: All declarations of 'PluginOptionsByType' must have identical type parameters.
3 participants