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

feat(types): improve generic types in specs, and spec prop types #1421

Merged
merged 21 commits into from
Jan 5, 2022

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Oct 7, 2021

Summary

Adds improved generic type for Datum in BarSeries, LineSeries, AreaSeries, BubbleSeries, BarSeries, HistogramBarSeries, Partition, Heatmap, GroupBy and LineAnnotation specs.

All spec components now export corresponding ___Props types (i.e. BarSeriesProps from BarSeries).

BREAKING CHANGE

The xAccessor and yAccessor props on all relevant specs have been removed in favor of explicit accessors derived from the generic Datum type. Stronger typing on data prop that may cause type errors.

Details

specComponentFactory

Arguments

  • overrides - Object containing overrides extended from the Spec type. These values are replaced on the Spec and are never exposed in the public API of the components. The props chartType and specType are required. All values listed in overrides are excluded from being used a defaults.
  • defaults - Object containing default values extended from the Spec. Values provided will be forced as optional on the Spec regardless of the optionality of the type on the original Spec type. These values will only be used when their respective prop on the spec component is not defined, this is in the form of a spread operator.

Note: These arguments should NOT be typed, these are best if defined directly inside the arguments as an object literal and not passed as a variable. This is to leverage the implicit typed values built into the method.

The specComponentFactory method has been refactored to automatically infer the types of the final component props given the spec type, the overridden props and the default props. An example of this is shown below.

export const Axis = specComponentFactory<AxisSpec>()(
  {
    chartType: ChartType.XYAxis,
    specType: SpecType.Axis,
  },
  {
    groupId: DEFAULT_GLOBAL_ID,
    hide: false,
    showOverlappingTicks: false,
    showOverlappingLabels: false,
    position: Position.Left,
  },
);

export type AxisProps = ComponentProps<typeof Axis>;

Note: the function is initially invoked with no arguments (i.e. specComponentFactory<AxisSpec>()). This is done because typescript only infers generics if none are defined, as soon as one is defined it expects all to be defined. Thus I do this because I want to defined the Spec type but I want to infer all the other argument generics.

image

buildSFProps

Arguments

  • overrides - See specComponentFactory description
  • defaults - See specComponentFactory description

This method is used to extract the implicit typings for a given spec definition outside of the specComponentFactory method. But why? The way typescript allows react component props to be driven my generics requires a top level function component that defines the generic types and props that consume these generics. Thus specComponentFactory would not work to defined generics that are unique to each Spec.

The BuildProps interface is used to define the props given the overrides and defaults. You will see that the final type includes optionals and requires props but the note says it's deprecated. I did this to warn against using these as values because these are simply used to share the inferred types as we do not know ahead of time what optional or required props will be passed but we can infer what their types are.

export interface BuildProps<
  S extends Spec,
  Overrides extends SFOverrideKeys<S>,
  Defaults extends SFDefaultKeys<S, Overrides>,
  Optionals extends SFOptionalKeys<S, Overrides, Defaults>,
  Requires extends SFRequiredKeys<S, Overrides, Defaults, Optionals>
> {
  overrides: SFOverrides<S, Overrides>;
  defaults: SFDefaults<S, Overrides, Defaults>;
  /** @deprecated typing only do not use as value */
  optionals: Pick<S, Optionals>;
  /** @deprecated typing only do not use as value */
  requires: Pick<S, Requires>;
}

useSpecFactory

Arguments

  • props - The fully built out props including overrides, defaults, optionals and required values.

This react hook is a the magic behind the spec parser. The logic within this hook used to be called by implementing the same logic inside the factory-created spec component. This was fine but this required calling getConnect()(MySpecFactory) everywhere it was used. At the time react-redux connect was used to connect the spec component to the redux store using the pure option.

The used of pure option is no longer required as react-redux has since made pure the default behavior across the board, see code.

// reduxjs/react-redux/blob/master/src/components/connect.tsx
if (process.env.NODE_ENV !== 'production') {
  if (pure !== undefined) {
    throw new Error(
      'The `pure` option has been removed. `connect` is now always a "pure/memoized" component'
    )
  }
}

With this change we can now use the useDispatch hook in place of the connect method for a simplified usage.

Thus creating a spec factory component with implied generic types would look something like...

const buildProps = buildSFProps<BarSeriesSpec<unknown>>()(
  {
    chartType: ChartType.XYAxis,
    specType: SpecType.Series,
    seriesType: SeriesType.Bar,
  },
  {
    groupId: DEFAULT_GLOBAL_ID,
    xScaleType: ScaleType.Ordinal,
    yScaleType: ScaleType.Linear,
    hideInLegend: false,
    enableHistogramMode: false,
  },
);

export const BarSeries = function <Datum extends BaseDatum>(
  props: SFProps<
    BarSeriesSpec<Datum>,
    keyof typeof buildProps['overrides'],
    keyof typeof buildProps['defaults'],
    keyof typeof buildProps['optionals'],
    keyof typeof buildProps['requires']
  >,
) {
  const { defaults, overrides } = buildProps;
  useSpecFactory<BarSeriesSpec<Datum>>({ ...defaults, ...props, ...overrides });
  return null;
};

export type BarSeriesProp = ComponentProps<typeof BarSeries>;

This setup, defining buildProps before the spec with unknown for the generic, can conflict with types and you may need to define the buildProps within the spec component function to access the correct Datum type. In such case, use the useRef hook to only call this once. See packages/charts/src/chart_types/heatmap/specs/heatmap.ts for an example of this.

Now the shape of the data prop is used as the Datum type across the BarSeries props.

Screen.Recording.2021-10-07.at.07.29.12.PM.mp4

The generic type for these spec components are implicit by default but can be explicitly defined as well such as...

<BarSeries<{ x: number; y: number }>
  id="bars"
  xAccessor="x"
  yAccessors={['y']}
  data={[
    { x: 0, y: 2 },
    { x: 1, y: 7 },
  ]}
/>

Datums as tuple arrays are also allowable, though they are treated as an array without knowing the limits of the array, thus the datum keys are simply number and not 'x' | 'y', etc. When the datum type is not provided via generic, the accessors fallback to the current string | number type.

Limitations

xAccessor and yAccessors could be forced to be a key of the data but this would be overly restrictive based on current chart usage. Thus, for now this will remain number | string in addition to the keyof Datum however the loose typing will not show the keyof Datum values in the type.

generic types cannot be shared between siblings, as of my current understanding. Thus Settings and other props that currently use Datum as any will remain.

This does NOT allow for generic types to be used in the vast majority of the source logic because of the separation of the react components and the redux store. Thus the generics will still fallback to a datum type of any.

Issues

Related to #1380 and #547

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios

@nickofthyme nickofthyme added enhancement New feature or request :specs Chart specifications related issue API Changes the external API types :all Applies to all chart types labels Oct 7, 2021
@nickofthyme nickofthyme marked this pull request as ready for review October 15, 2021 21:45
@nickofthyme
Copy link
Collaborator Author

@elasticmachine merge upstream

@nickofthyme
Copy link
Collaborator Author

@markov00 can you review this one please 🙏🏼

@nickofthyme nickofthyme merged commit 562929e into elastic:master Jan 5, 2022
@nickofthyme nickofthyme deleted the spec-typings branch January 5, 2022 21:06
nickofthyme pushed a commit that referenced this pull request Jan 5, 2022
# [42.0.0](v41.0.1...v42.0.0) (2022-01-05)

### Bug Fixes

* **flamegraph:** solve animation regression occurring with 6db2677 ([#1541](#1541)) ([5ec6037](5ec6037)), closes [#1540](#1540)
* **heatmap:** render empty state ([#1532](#1532)) ([59002df](59002df))
* **waffle:** fix strange 0 text in legend item extra when label is 0 ([#1538](#1538)) ([72224b9](72224b9))

### Features

* **goal:** add valueFormatter for tooltip ([#1529](#1529)) ([8139973](8139973))
* **heatmap:** add axis titles ([#1503](#1503)) ([a87325d](a87325d))
* **types:** improve generic types in specs, and spec prop types ([#1421](#1421)) ([562929e](562929e))

### BREAKING CHANGES

* **types:** The `xAccessor` and `yAccessor` are now required on all xy chart specs. Stronger typing on `data` prop that may cause type errors when using untyped array (i.e. `const arr: never[] = []`). Other minor type changes related to spec types.
* **heatmap:** The heatmap yAxisLabel.padding style type is changed from Pixel | Partial<Padding> to Pixels | Padding. The heatmap axis labels are now correctly subjected to padding calculations and it will result in a slightly different position of labels.

Co-authored-by: Marco Vettorello <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:all Applies to all chart types API Changes the external API types enhancement New feature or request :specs Chart specifications related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants