ref(charts): improve chart types

[JonasBa]

Just noticed this, but I think that we have at some point made the mistake of merging the wide Series type with the SeriesXXXType (where xxx is line/bar etc)... You can see from the source, that we are omitting some properties (a clue that merging is not possible, which is confirmed when attempting to use an interface)

export type AreaChartSeries = Series & Omit<LineSeriesOption, 'data' | 'name'>;
// followed by 
export interface AreaChartProps extends Omit<ChartProps, 'series'> {
  series: AreaChartSeries
}

The issue here is that LineSeriesOption already extends Series, hence why it needs to be omitted (twice) in order to allow for the final series type to be set. I think this might also result in TS allowing users to set attributes onto the series object which may not actually do anything (depending on the chart they are using).

Seems like that part might require a larger overhaul that I dont intend on taking right now and I instead of focused on cleaning up the unnecessary type aliases

[billyvg]

Is this still necessary?

[JonasBa]

oh no, it's not. Let me remove it

[gggritso]

I think this looks great, just a few questions so I can get my brain around the changes:

• I see css is no longer omitted from the props, is that because React.ComponentProps was adding it, and you're exporting the prop interface instead? Or is there some other reason?
• Why omit color and id from props?

Also, as a general question, why export component props vs. using React.ComponentProps? From what I can see, ComponentProps make sure that React-y props like ref are correctly available

[JonasBa]

I think this looks great, just a few questions so I can get my brain around the changes:

• I see css is no longer omitted from the props, is that because React.ComponentProps was adding it, and you're exporting the prop interface instead? Or is there some other reason?
• Why omit color and id from props?

Also, as a general question, why export component props vs. using React.ComponentProps? From what I can see, ComponentProps make sure that React-y props like ref are correctly available

Great question.

When you import something via import {fn} from "src", you are importing the value of fn (+ any other module required by the module you are importing) and telling TS to infer from the value. The first major difference here is that when that happens, bundlers cannot tree shake and drop import {fn} from "src" at runtime because src may have side effects. If your code only requires the type, that means all the work following require chains is unnecessary. At best, you are evaluating at least one extra module in your require chain (in dev or test envs, this also means additional work being done by the build systems). Import type statements are however not part of the JS runtime and get stripped out at build time by typescript, meaning your module will never be imported and evaluated.

When you use React.ComponentProps, you are creating a new type alias which increases the number of TS symbols and types, meaning more memory usage for the compiler (how much depends on the type). When you reference a type, that is not necessary.

That is the major distinction, but there are smaller ones like developer ergonomic - when types are explicitly imported, you can go to definition in a single user action, vs when they are not, go to definition takes you to the value, and then you need to usually perform at least one extra action (scroll or click) to go to definition (if the type is defined in the same file). This might seem minor, but I would argue it is important as it minimizes the back and forth.

In all fairness, even though I listed the functional differences for importing props vs inferring them, I think the major benefit of importing types is that it makes the code explicit - this would probably be my main argument for why I'd advocate for it. Reading React.ComponentProps reads like I'm decoding the enigma compared to just knowing that its just some component props.

• Why omit color and id from props?
  We need to do this as it conflicts with the wider Series type - this is the same issue that I described in the PR description, we should have used the narrower TypeSeries for specific charts at the start, it would require a larger refactor now :/

For CSS omitting - if you Omit<Props, "does_not_exist">, TS will not warn you that the prop you are omitting does not exist and the final type will be correct - you can see here that CSS was not actually used anywhere hence why we have no errors. I removed it and it didn't produce any errors, so I 🔪 it :)

Btw, React.ComponentProps doesn't add any props, it is basically a react component type helper for function argument inference, the CSS is only added if your component is a styled component.

Sorry for the long reply lol, maybe it will serve someone else who might read it (I'll use it at a TSC to try and make the argument for type imports 😅)

[gggritso]

@JonasBa roger that, thanks for the explanation, checks out!