From 673a0558c12c128a80003bd04eef2ff11341cdbc Mon Sep 17 00:00:00 2001 From: dangreen Date: Tue, 19 Oct 2021 03:45:13 +0700 Subject: [PATCH] fix: fix components' props types fix #734, possible fix #741 --- .github/workflows/checks.yml | 20 +++++++ .size-limit | 4 +- package.json | 8 ++- src/chart.tsx | 73 ++++++++++++---------- src/index.tsx | 103 ++----------------------------- src/typedCharts.tsx | 27 +++++++++ src/types.ts | 59 ++++++++++++++---- stories/Bar.data.ts | 1 + test/chart.test-d.tsx | 38 ++++++++++++ test/chart.test.tsx | 113 +++++++++++++++-------------------- yarn.lock | 87 +++++++++++++++++++++++++-- 11 files changed, 322 insertions(+), 211 deletions(-) create mode 100644 src/typedCharts.tsx create mode 100644 test/chart.test-d.tsx diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 44b5830e7..2bb0728dc 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -17,3 +17,23 @@ jobs: uses: andresz1/size-limit-action@v1 with: github_token: ${{ secrets.GITHUB_TOKEN }} + typings: + runs-on: ubuntu-latest + name: Checking typings + if: "!contains(github.event.head_commit.message, '[ci skip]')" + steps: + - name: Checkout the repository + uses: actions/checkout@v2 + - name: Install Node.js + uses: actions/setup-node@v2 + with: + node-version: 12 + - name: Install dependencies + uses: bahmutov/npm-install@v1 + with: + install-command: yarn --frozen-lockfile --ignore-engines + - name: Prebuild + run: yarn build + - name: Check typings + if: success() + run: yarn test:typings diff --git a/.size-limit b/.size-limit index fecfbb369..3d85937f6 100644 --- a/.size-limit +++ b/.size-limit @@ -8,7 +8,7 @@ { "path": "dist/index.js", "limit": "9.4 KB", - "import": "ChartComponent" + "import": "Chart" }, { "path": "dist/index.modern.js", @@ -19,6 +19,6 @@ { "path": "dist/index.modern.js", "limit": "9.4 KB", - "import": "ChartComponent" + "import": "Chart" } ] diff --git a/package.json b/package.json index 69ac7001d..294e8cd72 100644 --- a/package.json +++ b/package.json @@ -4,6 +4,7 @@ "description": "React components for Chart.js", "main": "dist/index.js", "module": "dist/index.modern.js", + "types": "dist/index.d.ts", "author": "Jeremy Ayerst", "homepage": "https://github.com/reactchartjs/react-chartjs-2", "license": "MIT", @@ -31,7 +32,8 @@ "test:unit": "jest -c jest.config.json", "test:build": "yarn build", "test:size": "size-limit", - "test": "yarn test:lint && yarn test:unit && yarn test:build && yarn test:size", + "test:typings": "tsd", + "test": "yarn test:lint && yarn test:unit && yarn test:build", "format": "prettier --write src", "predeploy": "cd example && yarn && yarn build", "deploy": "gh-pages -d example/build", @@ -96,9 +98,13 @@ "simple-git-hooks": "^2.6.1", "size-limit": "^6.0.3", "standard-version": "^9.3.1", + "tsd": "^0.18.0", "typescript": "^4.4.3", "webpack": "^5.58.2" }, + "tsd": { + "directory": "./test" + }, "files": [ "dist" ] diff --git a/src/chart.tsx b/src/chart.tsx index 3e4e334b0..fea02fe8b 100644 --- a/src/chart.tsx +++ b/src/chart.tsx @@ -6,35 +6,43 @@ import React, { useMemo, forwardRef, } from 'react'; -import Chart from 'chart.js/auto'; -import type { ChartData } from 'chart.js'; +import type { Ref, MouseEvent } from 'react'; +import ChartJS from 'chart.js/auto'; +import type { ChartData, ChartType, DefaultDataPoint } from 'chart.js'; import merge from 'lodash/merge'; import assign from 'lodash/assign'; import find from 'lodash/find'; -import { Props } from './types'; +import { Props, ChartJSOrUndefined, TypedChartComponent } from './types'; -const ChartComponent = forwardRef((props, ref) => { - const { - id, - className, +function ChartComponent< + TType extends ChartType = ChartType, + TData = DefaultDataPoint, + TLabel = unknown +>( + { height = 150, width = 300, redraw = false, type, data, - options = {}, + options, plugins = [], getDatasetAtEvent, getElementAtEvent, getElementsAtEvent, fallbackContent, - ...rest - } = props; + onClick: onClickProp, + ...props + }: Props, + ref: Ref> +) { + type TypedChartJS = ChartJSOrUndefined; + type TypedChartData = ChartData; const canvas = useRef(null); - const computedData = useMemo(() => { + const computedData = useMemo(() => { if (typeof data === 'function') { return canvas.current ? data(canvas.current) @@ -44,17 +52,15 @@ const ChartComponent = forwardRef((props, ref) => { } else return merge({}, data); }, [data, canvas.current]); - const [chart, setChart] = useState(); + const [chart, setChart] = useState(); - useImperativeHandle(ref, () => chart, [ - chart, - ]); + useImperativeHandle(ref, () => chart, [chart]); const renderChart = () => { if (!canvas.current) return; setChart( - new Chart(canvas.current, { + new ChartJS(canvas.current, { type, data: computedData, options, @@ -63,38 +69,42 @@ const ChartComponent = forwardRef((props, ref) => { ); }; - const onClick = (e: React.MouseEvent) => { + const onClick = (event: MouseEvent) => { + if (onClickProp) { + onClickProp(event); + } + if (!chart) return; getDatasetAtEvent && getDatasetAtEvent( chart.getElementsAtEventForMode( - e as unknown as Event, + event.nativeEvent, 'dataset', { intersect: true }, false ), - e + event ); getElementAtEvent && getElementAtEvent( chart.getElementsAtEventForMode( - e as unknown as Event, + event.nativeEvent, 'nearest', { intersect: true }, false ), - e + event ); getElementsAtEvent && getElementsAtEvent( chart.getElementsAtEventForMode( - e as unknown as Event, + event.nativeEvent, 'index', { intersect: true }, false ), - e + event ); }; @@ -128,8 +138,10 @@ const ChartComponent = forwardRef((props, ref) => { if (!currentDataSet || !newDataSet.data) return { ...newDataSet }; if (!currentDataSet.data) { + // @ts-expect-error Need to refactor currentDataSet.data = []; } else { + // @ts-expect-error Need to refactor currentDataSet.data.length = newDataSet.data.length; } @@ -163,23 +175,20 @@ const ChartComponent = forwardRef((props, ref) => { } else { updateChart(); } - }, [props, computedData]); + }); return ( {fallbackContent} ); -}); +} -export default ChartComponent; +export const Chart = forwardRef(ChartComponent) as TypedChartComponent; diff --git a/src/index.tsx b/src/index.tsx index 4b169df99..377d0a09e 100644 --- a/src/index.tsx +++ b/src/index.tsx @@ -1,98 +1,7 @@ -import React, { forwardRef } from 'react'; -import Chart from 'chart.js/auto'; -import { defaults } from 'chart.js'; +// @todo Remove these exports +export { default as Chart } from 'chart.js/auto'; +export { defaults } from 'chart.js'; -import { Props } from './types'; -import ChartComponent from './chart'; - -export const Line = forwardRef>( - (props, ref) => ( - - ) -); - -export const Bar = forwardRef>( - (props, ref) => ( - - ) -); - -export const Radar = forwardRef>( - (props, ref) => ( - - ) -); - -export const Doughnut = forwardRef>( - (props, ref) => ( - - ) -); - -export const PolarArea = forwardRef>( - (props, ref) => ( - - ) -); - -export const Bubble = forwardRef>( - (props, ref) => ( - - ) -); - -export const Pie = forwardRef>( - (props, ref) => ( - - ) -); - -export const Scatter = forwardRef>( - (props, ref) => ( - - ) -); - -export { Chart, defaults }; - -export default ChartComponent; +// @todo Make named export instead of default +export { Chart as default } from './chart'; +export * from './typedCharts'; diff --git a/src/typedCharts.tsx b/src/typedCharts.tsx new file mode 100644 index 000000000..da17a7d86 --- /dev/null +++ b/src/typedCharts.tsx @@ -0,0 +1,27 @@ +import React, { forwardRef } from 'react'; +import { ChartType } from 'chart.js'; + +import { Props, ChartJSOrUndefined, TypedChartComponent } from './types'; +import { Chart } from './chart'; + +function createTypedChart(type: T) { + return forwardRef, Omit, 'type'>>( + (props, ref) => + ) as TypedChartComponent; +} + +export const Line = createTypedChart('line'); + +export const Bar = createTypedChart('bar'); + +export const Radar = createTypedChart('radar'); + +export const Doughnut = createTypedChart('doughnut'); + +export const PolarArea = createTypedChart('polarArea'); + +export const Bubble = createTypedChart('bubble'); + +export const Pie = createTypedChart('pie'); + +export const Scatter = createTypedChart('scatter'); diff --git a/src/types.ts b/src/types.ts index 24abf923f..349b56759 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1,23 +1,33 @@ -import type { CanvasHTMLAttributes, ReactNode, MouseEvent } from 'react'; import type { + CanvasHTMLAttributes, + ForwardedRef, + ReactNode, + MouseEvent, +} from 'react'; +import type { + Chart, ChartType, ChartData, ChartOptions, + DefaultDataPoint, Plugin, InteractionItem, } from 'chart.js'; -export interface Props extends CanvasHTMLAttributes { - id?: string; - className?: string; - height?: number; - width?: number; +export interface Props< + TType extends ChartType = ChartType, + TData = DefaultDataPoint, + TLabel = unknown, + TOtherType extends TType = TType +> extends CanvasHTMLAttributes { + type: TType; + data: + | ChartData + | ((canvas: HTMLCanvasElement) => ChartData); + options?: ChartOptions; + plugins?: Plugin[]; redraw?: boolean; - type: ChartType; - data: ChartData | ((canvas: HTMLCanvasElement) => ChartData); - options?: ChartOptions; fallbackContent?: ReactNode; - plugins?: Plugin[]; getDatasetAtEvent?: ( dataset: InteractionItem[], event: MouseEvent @@ -31,3 +41,32 @@ export interface Props extends CanvasHTMLAttributes { event: MouseEvent ) => void; } + +/** + * @todo Replace `undefined` with `null` + */ +export type ChartJSOrUndefined< + TType extends ChartType = ChartType, + TData = DefaultDataPoint, + TLabel = unknown +> = Chart | undefined; + +export type TypedChartComponent< + TDefaultType extends ChartType = ChartType, + TOmitType = false +> = TOmitType extends true + ? , TLabel = unknown>( + props: Omit, 'type'> & { + ref?: ForwardedRef>; + } + ) => JSX.Element + : < + TType extends ChartType = ChartType, + TData = DefaultDataPoint, + TLabel = unknown, + TOtherType extends TType = TType + >( + props: Props & { + ref?: ForwardedRef>; + } + ) => JSX.Element; diff --git a/stories/Bar.data.ts b/stories/Bar.data.ts index 1e879c01a..e4aec5253 100644 --- a/stories/Bar.data.ts +++ b/stories/Bar.data.ts @@ -110,6 +110,7 @@ export const groupedOptions = { }, responsive: true, interaction: { + mode: 'index', intersect: false, }, scales: { diff --git a/test/chart.test-d.tsx b/test/chart.test-d.tsx new file mode 100644 index 000000000..5daf34895 --- /dev/null +++ b/test/chart.test-d.tsx @@ -0,0 +1,38 @@ +import { expectError } from 'tsd'; +import React from 'react'; +import { Plugin } from 'chart.js'; +import Chart, { Scatter, Doughnut } from '../src'; + +const data = { + datasets: [], +}; + +/** + * Should check type-specific props + */ + +[]} />; +[]} />; + +expectError([]} />); +expectError([]} />); + +/** + * Should check type-specific options + */ + +; + +expectError( + +); diff --git a/test/chart.test.tsx b/test/chart.test.tsx index baa8e4d82..e04ba419b 100644 --- a/test/chart.test.tsx +++ b/test/chart.test.tsx @@ -1,9 +1,9 @@ import React from 'react'; import { render, cleanup, fireEvent } from '@testing-library/react'; -import Chart from 'chart.js/auto'; -import ChartComponent from '../src/chart'; +import ChartJS from 'chart.js/auto'; +import Chart from '../src'; -describe('', () => { +describe('', () => { const data = { labels: ['red', 'blue'], datasets: [{ label: 'colors', data: [1, 2] }], @@ -14,7 +14,7 @@ describe('', () => { }; let chart: any, update: any, destroy: any; - const ref = (el: Chart | null): void => { + const ref = (el: ChartJS | undefined | null): void => { chart = el; if (chart) { @@ -37,9 +37,7 @@ describe('', () => { }); it('should not pollute props', () => { - render( - - ); + render(); expect(data).toStrictEqual({ labels: ['red', 'blue'], @@ -52,18 +50,14 @@ describe('', () => { }); it('should set ref to chart instance', () => { - render( - - ); + render(); expect(chart).toBeTruthy(); - expect(chart instanceof Chart).toBe(true); + expect(chart instanceof ChartJS).toBe(true); }); it('should pass props onto chart', () => { - render( - - ); + render(); expect(chart.config.data).toMatchObject(data); expect(chart.config.options).toMatchObject(options); @@ -71,12 +65,16 @@ describe('', () => { }); it('should pass props onto chart if data is fn', () => { - const dataFn = jest.fn(c => (c ? data : {})); - - render( - + const dataFn = jest.fn(c => + c + ? data + : { + datasets: [], + } ); + render(); + expect(chart.config.data).toMatchObject(data); expect(chart.config.options).toMatchObject(options); expect(chart.config.type).toEqual('bar'); @@ -93,15 +91,13 @@ describe('', () => { }; const { rerender } = render( - + ); // const meta = chart.config.data.datasets[0]._meta; const id = chart.id; - rerender( - - ); + rerender(); expect(chart.config.data).toMatchObject(newData); // make sure that other properties were maintained @@ -117,15 +113,13 @@ describe('', () => { }; const { rerender } = render( - + ); const meta = chart.config.data.datasets[0]._meta; const id = chart.id; - rerender( - - ); + rerender(); expect(chart.config.data).toMatchObject(newData); expect(meta).not.toEqual(chart.config.data.datasets[0]); @@ -151,16 +145,14 @@ describe('', () => { }; const { rerender } = render( - + ); const meta = Object.assign({}, chart._metasets); const id = chart.id; - rerender( - - ); + rerender(); expect(chart.config.data).toMatchObject(newData); expect(meta[0]).toBe(chart._metasets[1]); @@ -172,7 +164,10 @@ describe('', () => { it('should properly update when original data did not exist', () => { const oldData = { labels: ['red', 'blue'], - datasets: [{ label: 'new-colors' }, { label: 'colors', data: [3, 2] }], + datasets: [ + { label: 'new-colors', data: [] }, + { label: 'colors', data: [3, 2] }, + ], }; const newData = { @@ -184,7 +179,7 @@ describe('', () => { }; const { rerender } = render( - + ); // even when we feed the data as undefined, the constructor will @@ -194,9 +189,7 @@ describe('', () => { const id = chart.id; - rerender( - - ); + rerender(); expect(chart.config.data).toMatchObject(newData); expect(meta[0]).toBe(chart._metasets[1]); @@ -215,18 +208,19 @@ describe('', () => { const newData = { labels: ['red', 'blue'], - datasets: [{ label: 'colors', data: [4, 5] }, { label: 'new-colors' }], + datasets: [ + { label: 'colors', data: [4, 5] }, + { label: 'new-colors', data: [] }, + ], }; const { rerender } = render( - + ); const id = chart.id; - rerender( - - ); + rerender(); expect(chart.config.data).toMatchObject(newData); expect(update).toHaveBeenCalled(); @@ -239,14 +233,12 @@ describe('', () => { }; const { rerender } = render( - + ); const id = chart.id; - rerender( - - ); + rerender(); expect(chart.options).toMatchObject(newOptions); expect(update).toHaveBeenCalled(); @@ -260,26 +252,14 @@ describe('', () => { }; const { rerender } = render( - + ); // const id = chart.id; const originalChartDestroy = Object.assign({}, destroy); rerender( - + ); expect(originalChartDestroy).toHaveBeenCalled(); @@ -287,7 +267,7 @@ describe('', () => { it('should destroy when unmounted', () => { const { unmount } = render( - + ); expect(chart).toBeTruthy(); @@ -299,7 +279,7 @@ describe('', () => { it('should add className ', () => { render( - ', () => { const getDatasetAtEvent = jest.fn(); const { getByTestId } = render( - ', () => { const getElementAtEvent = jest.fn(); const { getByTestId } = render( - ', () => { const getElementsAtEvent = jest.fn(); const { getByTestId } = render( - ', () => { it('should show fallback content if given', () => { const fallback =

Fallback content

; const { getByTestId } = render( - ', () => { it('should pass through aria labels to the canvas element', () => { const ariaLabel = 'ARIA LABEL'; render( -