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

Change and reorder the types definitions #759

Merged
merged 3 commits into from
Jun 8, 2022
Merged

Change and reorder the types definitions #759

merged 3 commits into from
Jun 8, 2022

Conversation

stockiNail
Copy link
Collaborator

This PR is changing the types definitions in order to:

  • have all types on top of the file
  • remove AnnotationCoordinates because adds the options in CoreAnnotationOptions
  • remove AnnotationPluginCommonOptions and changes common option defintion to accept a types annotation options
  • reorder the options by their name, sorting alphabetically

@stockiNail stockiNail added the types Typescript type changes label Jun 6, 2022
@stockiNail stockiNail added this to the 2.0.0 milestone Jun 6, 2022
@stockiNail stockiNail changed the title Changes and reorder the types definitions Change and reorder the types definitions Jun 6, 2022
Copy link
Collaborator

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

Not sure what the reason was behind re exporting string as another type without actually doing something with it but I think this is better and more type safe that what it was before

types/label.d.ts Outdated Show resolved Hide resolved
@stockiNail
Copy link
Collaborator Author

Not sure what the reason was behind re exporting string as another type without actually doing something with it but I think this is better and more type safe that what it was before

@LeeLenaleee we have tried in the past that format but it seems is not compatible with some TS version... I'm looking for the comment in a PR

Co-authored-by: Jacco van den Berg <[email protected]>
@LeeLenaleee
Copy link
Collaborator

LeeLenaleee commented Jun 8, 2022

Guess it is anything lower as 4.1 @stockiNail.
Because that is when it was released: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-1.html

But since it is a V2 release I guess we can call it a breaking change and require at least TS version 4.1 since 4.1 is already 2 years old

image

EDIT:
Guess we also need to put a note in the migration guide then

@stockiNail
Copy link
Collaborator Author

stockiNail commented Jun 8, 2022

Guess it is anything lower as 4.1 @stockiNail. Because that is when it was released: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-1.html

But since it is a V2 release I guess we can call it a breaking change and require at least TS version 4.1 since 4.1 is already 2 years old

image

Could make sense for me. If all agree, I need to change this PR in order to

  • remove the tests on previous TS 4.1 version
  • add a note in the migration guide
  • add tag as breaking change

Let me know. As said, for me it's fine.

@stockiNail stockiNail merged commit 5c4e8dd into chartjs:master Jun 8, 2022
@stockiNail stockiNail deleted the fixTypesDef branch June 8, 2022 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change types Typescript type changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants