Add typescript definitions

[anajavi]

what?

This adds typescript definition for the library. Not complete, but all series types should have types.

References highcharts types.

Somewhat related to discussion in #113 and in #267 (comment)

why?

• Better IDE experience even when writing plain js.
• Type checking for those who use typescript

caveats

• Event handlers are not typed, instead the evented components accept any props in addition to highcharts options

I am no way expert in writing definition files. I suggest that we release this and see what happens? It's easy to remove if it causes too much a headache. Maybe adding something in the release notes about "beta types".

[codecov-commenter]

Codecov Report

Merging #303 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #303   +/-   ##
=======================================
  Coverage   88.27%   88.27%           
=======================================
  Files          82       82           
  Lines        1049     1049           
  Branches      206      206           
=======================================
  Hits          926      926           
  Misses        107      107           
  Partials       16       16           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 357cbee...7bf514b. Read the comment docs.

[veloek]

We've been maintaining our own typings for (a subset of) RJH, so this is really great!

You haven't considered moving the codebase towards typescript? I realise it's quite some work to get there, but I think it would be worth while. If not, please make sure to keep the typings up-to-date. Misleading types are worse than no types.

[anajavi]

We've been maintaining our own typings for (a subset of) RJH, so this is really great!

Can you please paste this definition to your module declaration and report if it works for you?

You haven't considered moving the codebase towards typescript? I realise it's quite some work to get there, but I think it would be worth while.

That's up to whawker to decide. Previously typescript was quite cumbersome to use with react, but latest releases have been better. It's however quite an effort and adds complexity.

[veloek]

Can you please paste this definition to your module declaration and report if it works for you?

We're using the react-jsx-highstock package and the highstock specific components are missing, but other than that it seems to work. There are a few any types though, which could be improved on further.

[anajavi]

There are a few any types though, which could be improved on further.

Do you happen to have definitions for those?

[veloek]

Well, I would have to go through the components and look at the code. One simple example is Credits where the props type should be Highcharts.CreditsOptions & { enabled?: boolean }.

[anajavi]

Well, I would have to go through the components and look at the code. One simple example is Credits where the props type should be Highcharts.CreditsOptions & { enabled?: boolean }.

I added type for <Credits /> in 19a4c68

[veloek]

Nice @anajavi, this is looking good! I would like to have the event handlers listed out though, instead of [x: string]: any, so they also get their appropriate types.

[anajavi]

Nice @anajavi, this is looking good! I would like to have the event handlers listed out though, instead of [x: string]: any, so they also get their appropriate types.

Something like this should probably work:

  type ChartProps = {
    onSelection?: Highcharts.ChartSelectionCallbackFunction;
    [x: string]: any; // TODO: this is here to allow eventhandlers like onAfterAddSeries
  } & Partial<Highcharts.ChartOptions>;

  export function Chart(props: ChartProps): ReactElement;

There are however more events in highcharts than there are in their typings. One example is afterAddSeries.

[anajavi]

Nice @anajavi, this is looking good! I would like to have the event handlers listed out though, instead of [x: string]: any, so they also get their appropriate types.

@veloek I added the event handler typings if you like to try.

[veloek]

@anajavi Great! Seems to work just fine :-)

Just wondering about the highstock package. Will that one get these typings? It will need some of it's own as well, but for typescript to figure out where to look based on the import, all types must be present in the highstock package as well.

[anajavi]

@anajavi Great! Seems to work just fine :-)

Just wondering about the highstock package. Will that one get these typings? It will need some of it's own as well, but for typescript to figure out where to look based on the import, all types must be present in the highstock package as well.

I think we should land react-jsx-highcharts types first. When the types have been released it is easier for somebody(you?) to make a PR for highstock.

[whawker]

You haven't considered moving the codebase towards typescript?

To be honest I don't know Typescript, so I would struggle to maintain the project. It is something I intend to learn but I don't use it day-to-day currently, like I do with JS

I suggest that we release this and see what happens?

Worth releasing as a beta first?

[anajavi]

Worth releasing as a beta first?

This certainly won't break anybodys application, but might confuse a bit. Maybe a beta release and mention in the #259 might get some feedback?

I intend to add some kind of test or eslint configuration for this later so we see if new highcharts release breaks the typings(most likely the eventhandlers).

[whawker]

Apologies for the insane delay on this, life during Covid just ends up trying to keep way too many balls in the air at once.

I've published a beta react-jsx-highcharts@4.2.0-beta.1 containing the types added here.

Would those of you interested (@anajavi, @veloek) mind giving them a whirl?

[anajavi]

Works nicely at my end.