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

Type fixes #7759

Merged
merged 3 commits into from
Sep 6, 2020
Merged

Type fixes #7759

merged 3 commits into from
Sep 6, 2020

Conversation

emmcb
Copy link
Contributor

@emmcb emmcb commented Sep 5, 2020

Some issues I found in types when migrating my typescript project to V3 beta:

  • When we do not know the data point type, set unknown instead of guessing it is number. Putting number here prevent the user to cast to the actual data type, with a typescript error that number is not compatible and thus cannot be casted.
  • As per the docs, we can assign new option object at runtime chart.options = {...}, so remove readonly modifier
  • parsing options is not only a dataset option, it can also be given in the root chart options, so add it to IChartOptions
  • ItooltipItem dataset property type should be IChartDataset
  • ILinearScaleOptions should inherit from ICartesianScaleOptions (linear scale is a cartesian scale)
  • tooltipFormat property were missing from time scale options

@emmcb
Copy link
Contributor Author

emmcb commented Sep 5, 2020

Added one commit to fix axis property also missing on tooltip option.

@etimberg
Copy link
Member

etimberg commented Sep 6, 2020

@sgratzl @kurkle thoughts on using unknown here? All of the other changes made sense to me, but for this one I'm not sure. It almost seems better to use a union type of the things we knowingly support number | string | Point

@kurkle
Copy link
Member

kurkle commented Sep 6, 2020

number | string | Point is not sufficient through. We also support dates and arrays in float bars. The Point notation also supports numer/string/date/array of these for x/y or custom property. Commonly the data objects contain extra info for tooltips etc.
I'm not a ts user, but to me unknown here might make sense here.

@etimberg
Copy link
Member

etimberg commented Sep 6, 2020

Good point on dates. Perhaps this is the least painful way to do it though users can accidentally pass bad data (e.g. the value is booleans and something in the chart will cast it to a number). For the custom properties part, that's probably where one would want to use a custom type.

@emmcb
Copy link
Contributor Author

emmcb commented Sep 6, 2020

Most of my charts are time chart with custom {t, y} data points. While when creating chart I can use generics to give my custom DataPoint type, there are still some places like tooltip callback where we lose this and get back an abstract chart and dataset types. In these cases unknown is I think the best way to not make any guess and force the user to do the cast.

@xr0master
Copy link
Contributor

xr0master commented Sep 6, 2020

Hello. Before it's merged, can I added a few issues with type?

  1. IChartConfiguration doesn't have plugins, probably it should be plugins?: IPlugin[],
  2. I think data also doesn't need the readonly flag, if we want to change all data.
  3. In interface Chart the update method may have UpdateMode type instead of string.
  4. What about creating units for a chart type instead of string? It may be like this:
type ChartTypePresets =
  | 'line'
  | 'bar'
  | 'radar'
  | 'doughnut'
  | 'pie'
  | 'polarArea'
  | 'bubble'
  | 'scatter';
type: ChartTypePresets | string; // string is for custom type

P.S. or shall I create own PR for these changes after this PR?

@etimberg
Copy link
Member

etimberg commented Sep 6, 2020

@xr0master let's discuss those changes in their own PR

@etimberg etimberg merged commit c2fc8a4 into chartjs:master Sep 6, 2020
@etimberg etimberg added this to the Version 3.0 milestone Sep 6, 2020
@emmcb emmcb deleted the type-fixes branch September 9, 2020 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants