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

Invalid typing probably? #901

Open
aafanaseva-shell opened this issue Jul 6, 2023 · 3 comments
Open

Invalid typing probably? #901

aafanaseva-shell opened this issue Jul 6, 2023 · 3 comments
Labels
types Typescript type changes

Comments

@aafanaseva-shell
Copy link

Hello again!

I saw lot's of usage of EventContext.element here and there in examples and tests.
e.g. [here](https://github.com/chartjs/chartjs-plugin-annotation/blob/ebd5c6f9c2f56dcf1cf213aa7af2a9e93d38059f/test/fixtures/ellipse/label-dynamic.js#L34C1-L35C1:

element.label.options.display = true;

But in typings I see that label is marked as AnnotationElement here.
Idk, did I understand it correctly, but I believe it should be CoreLabelOptions or smth similar?

I believe typing checker or eslint passes successfully because lot's of properties have the same name. But if you try to replace the code in the example above to:

element.label.options.content= "new label text";

you'll see an error since element.label.options is not type of AnnotationOptions

But maybe that's just things are mixed up in my head a bit 🙂

@stockiNail
Copy link
Collaborator

@aafanasev-shell The label property is marked correctly as AnnotationElement because the "inner" labels of the annotations are sub-element, in other words a label annotation defined inside another annotation.

The AnnotationElement definition has got the options as AnnotationOptions which is the base type for the options for each annotation type.

To access to the label options, you should cast it to LabelAnnotationOptions:

const lblOpts = ctx.element.label.options as LabelAnnotationOptions;
lblOpts.content = 'ciao';

Maybe we could create a specific type for label property but a casting should be anyway needed (I'm not TS expert, sorry).

@stockiNail stockiNail added the types Typescript type changes label Jul 7, 2023
@aafanaseva-shell
Copy link
Author

Afaik in general if you have to typecast something in TS, something is wrong with typing in system that made it so TS can't infer the type 🤔

@stockiNail
Copy link
Collaborator

Could agree with you 😉
Even if we will create a spefici annotation element for the label with CoreLabelOptions type fo roptions property, a casting could be needed because the annotation, which contains a label, doesn't have all options of all others.

I think in the future the plugin will be written in TS and probably this will change. I also agree with you that in new major version a review of the types would be needed, maybe introducing generics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types Typescript type changes
Projects
None yet
Development

No branches or pull requests

2 participants