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

Issues importing plugins and TS module augmentation #11288

Closed
stockiNail opened this issue May 15, 2023 · 10 comments · Fixed by #11309
Closed

Issues importing plugins and TS module augmentation #11288

stockiNail opened this issue May 15, 2023 · 10 comments · Fixed by #11309

Comments

@stockiNail
Copy link
Contributor

Expected behavior

I'm expecting that importing the plugins, registering them and then configure them in chart options, the project is compiled and working

Current behavior

I see some issues TS module augmentation used to define the plugins.
The first issue was related to PR #11244 which is forcing the plugins to add the default type to avoid issues on TS compiling.

interface PluginOptionsByType<TType extends ChartType = ChartType>

This update has been applied to annotation plugin but there is some open points about this change: chartjs/chartjs-plugin-datalabels#376

This issue has been introduced by version 4.1.0, probably by PR #10963

Anyway, adding the default type, the compiler is complaining about the plugin options:

Object literal may only specify known properties, and 'annotation' does not exist in type '_DeepPartialObject<PluginOptionsByType<keyof ChartTypeRegistry, keyof ChartTypeRegistry>>'.

See issue in annotation plugin: chartjs/chartjs-plugin-annotation#886

The issue is not related to annotation itself because testing only zoom and datalabels, the issue remains.
It seems that the module augmentation is ignoring the update to the types after the first one, therefore it depends on the order of the imports.

Reproducible sample

Project published here: chartjs/chartjs-plugin-annotation#854 (comment)

Optional extra steps/info to reproduce

I changed chart.js version, using 4.3.0 and annotation 3.0.0. Furthermore I have changed index.d.ts of chartjs-zoom-plugin setting the default type.
Furthermore add annotation and zoom (as empty object) to plugins chart options.

Possible solution

No response

Context

No response

chart.js version

v4.3.0

Browser name and version

No response

Link to your project

No response

@vincentp
Copy link

I experience the same issue. I tried to update from 4.0.1 to 4.3.0 and it failed with the same typing issue. But the error starts to occur from version 4.1.0. I'm using zoom, annotations and custom made plugins.

@stockiNail
Copy link
Contributor Author

I have created a simple repo which can reproduce this issue.

https://github.com/stockiNail/chartjs-11288

Commands:

npm i
npm test

Console and issue:

> test
> node node_modules/typescript/bin/tsc index.ts

index.ts:21:7 - error TS2322: Type '{ annotation: {}; zoom: {}; }' is not assignable to type '_DeepPartialObject<PluginOptionsByType<"bar", keyof ChartTypeRegistry>>'.
  Object literal may only specify known properties, and 'annotation' does not exist in type '_DeepPartialObject<PluginOptionsByType<"bar", keyof ChartTypeRegistry>>'.

21       annotation: { },
         ~~~~~~~~~~~~~~~

  node_modules/chart.js/dist/types/index.d.ts:2915:3
    2915   plugins: PluginOptionsByType<TType>;
           ~~~~~~~
    The expected type comes from property 'plugins' which is declared here on type '_DeepPartialObject<CoreChartOptions<"bar"> & ElementChartOptions<"bar"> & PluginChartOptions<"bar"> & DatasetChartOptions<"bar"> & ScaleChartOptions<...> & BarControllerChartOptions>'


Found 1 error in index.ts:21

@stockiNail
Copy link
Contributor Author

I have spent time to find out the solution for this issue.

It seems (but not being a TS expert, not sure) if the augmentation is on a object which consists of re-export , module augmentation doesn't work.

Adding PluginOptionsByType to src/types.ts explicit export (row 10), sounds working.

I will do additional tests to be sure.

@stockiNail
Copy link
Contributor Author

stockiNail commented May 24, 2023

Let me recap.

From version 4.0.1 (where the module augmentation was working) to 4.1.0, the rollup config was changed removing rollup-plugin-dts.
Having a look to the output files, I saw that the d.ts produced by rollup plugin exported the plugin definitions by an explicit export (no by re-export).
From 4.1.0, the definition are re-exported by export * from (in types.ts which exports export * from './index.js';) and this is breaking the module augmentation.

Therefore adding adding PluginOptionsByType to src/types.ts explicit export (row 10), the issue is SOLVED.

This issue was also causing the other TS definition problem (@simonbrunel this is related to the PR chartjs/chartjs-plugin-datalabels#376 that I have submitted in datalabels and where you correctly shared your doubts):

Error: node_modules/chartjs-plugin-annotation/types/index.d.ts:6:13 - error TS2428: 
  All declarations of 'PluginOptionsByType' must have identical type parameters.6   
  interface PluginOptionsByType<TType extends ChartType> {

which was fixed (as workaround) adding the default type (see #11244, which should be reverted I think).

But now, I think there is another decision to take. I can submit a PR to fix this issue but there are other definitions that plugins/controller could augment and are not in a explicit export.
For instance, the zoom plugin is augmenting UpdateModeEnum which is not explicitly exported. That means if other plugin will augment as well, adding another update mode, this issue will occur again for UpdateModeEnum (I have tested it, failing, in the repo that I created for this issue). Another example is ElementOptionsByType (augmented by annotation plugin).

The following table is showing the augmented definitions, performed by plugins in chartjs org (in addition to PluginOptionsByType, of course):

Plugin Augmented Definitions
annotation ElementOptionsByType
datalabels ChartDatasetProperties
deferred -
zoom UpdateModeEnum

@etimberg @kurkle @simonbrunel @dangreen @LeeLenaleee what do you think? I think the best thing is to solve this issue as proposed above, and about the other defintions which can be augment, we can take time for a solution. Make sense?

@ghost
Copy link

ghost commented May 26, 2023

Great work on solving this!

@liondog
Copy link

liondog commented Jul 10, 2023

@etimberg, @kurkle Is there a release planned soon? I have been stuck with v3.8.1 for over a year now. This fix and #877 finally lets me migrate to 4.x :-).

@LeeLenaleee
Copy link
Collaborator

@liondog 4.3.1 Should be available in +- 30 minutes

@maxhellwig
Copy link

Sorry, I'm not sure if this is related, but after updating to 4.3.1 I'm still getting type errors related to the typings:

chart.handler.ts:110:68 - error TS2339: Property 'labels' does not exist on type 'false | _DeepPartialObject<LegendOptions<keyof ChartTypeRegistry>>'.
  Property 'labels' does not exist on type 'false'.

110       renderLegendItems(chartElement, chart.options.plugins.legend.labels.generateLabels(chart));
                                                                       ~~~~~~

This happens for all plugin configurations, even build-ins like tooltip and legend:
image

I'm using this versions right now:
image

Also to mention that ng2-charts is also involved in version 3.1.2.

When I'm downgrading chartjs to 3.9.1 I don't have any issues.

Does anyone has an idea?

@stockiNail
Copy link
Contributor Author

@maxhellwig no, the issue is related to #11420 and PR #11403. We are going to revert the PR soon.

@stockiNail
Copy link
Contributor Author

@maxhellwig FYI, PR to fix it is submitted. #11422

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants