From a1a78964c26a4f1b6cf4d9b51800730e636382c9 Mon Sep 17 00:00:00 2001 From: Jianchao Yang Date: Mon, 9 Mar 2020 12:45:33 -0700 Subject: [PATCH] fix(legacy-table): avoid React DOM (#392) * fix(legacy-table): avoid React DOM `jquery.datatables` will manipulate DOMs, sometimes remove them. In case of component being reloaded with updated props, React will not be able to find those removed nodes, causing a `cannot removeChild` error. Because of the the way to assign row keys, if table shape changes (add or remove columns), React may also have difficulty match the cached nodes via keys. In general it's a bad idea to directly manipulate React rendered DOM nodes, so we better just let `jquery.datatables` handle everything. In the future, once we removed `jquery.datatables`, a pure React component will not have such issues. * fix(legacy-table): handle the case when percentMetrics is null * fix(legacy-table): linting errors * refactor: use nimbus build * test(legacy-table): add React component tests * test(legacy-table): more sophisticated cases * fix: address PR #392 comments * chore(legacy-table): clean up tests case setups Not seeing the console.warn errors anymore. So cleaning it up. Previously it was from `` component, but since we have updated the test case to not use , we are good now. * fix(legacy-table): misleading comment --- plugins/superset-ui-plugins/package.json | 8 +- .../package.json | 3 +- .../src/ReactDataTable.tsx | 96 +++++++++------- .../src/transformProps.ts | 6 +- .../test/ReactDataTable.test.tsx | 57 +++++++++ .../test/testData.ts | 108 ++++++++++++++++++ .../legacy-plugin-chart-table/Stories.tsx | 77 ++++++++++--- plugins/superset-ui-plugins/scripts/build.js | 22 ++-- .../superset-ui-plugins/scripts/setupJest.js | 6 + plugins/superset-ui-plugins/yarn.lock | 12 ++ 10 files changed, 324 insertions(+), 71 deletions(-) create mode 100644 plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/test/ReactDataTable.test.tsx create mode 100644 plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/test/testData.ts create mode 100644 plugins/superset-ui-plugins/scripts/setupJest.js diff --git a/plugins/superset-ui-plugins/package.json b/plugins/superset-ui-plugins/package.json index adf2c982cc..096748b0f7 100644 --- a/plugins/superset-ui-plugins/package.json +++ b/plugins/superset-ui-plugins/package.json @@ -62,6 +62,7 @@ "fast-glob": "^3.0.1", "fs-extra": "^8.0.1", "husky": "^4.2.1", + "identity-obj-proxy": "^3.0.0", "jest-mock-console": "^1.0.0", "lerna": "^3.15.0", "lint-staged": "^10.0.7", @@ -102,7 +103,8 @@ "jest": { "timers": "real", "setupFilesAfterEnv": [ - "@airbnb/config-jest/enzyme" + "@airbnb/config-jest/enzyme", + "./scripts/setupJest.js" ], "coverageThreshold": { "global": { @@ -111,6 +113,10 @@ "lines": 1, "statements": 1 } + }, + "moduleNameMapper": { + "\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$": "/__mocks__/fileMock.js", + "\\.(css|less)$": "identity-obj-proxy" } }, "eslint": { diff --git a/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/package.json b/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/package.json index 39971d9b81..5347025d21 100644 --- a/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/package.json +++ b/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/package.json @@ -41,6 +41,7 @@ "@superset-ui/time-format": "^0.12.0", "@superset-ui/translation": "^0.12.0", "jquery": "^3.4.1", - "react": "^16.8.0" + "react": "^16.8.0", + "react-dom": "^16.8.0" } } diff --git a/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx b/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx index e36d767095..eb28f3df8e 100644 --- a/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx +++ b/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/src/ReactDataTable.tsx @@ -18,6 +18,7 @@ */ import { t } from '@superset-ui/translation'; import React, { useEffect, createRef } from 'react'; +import ReactDOMServer from 'react-dom/server'; import { formatNumber, NumberFormats } from '@superset-ui/number-format'; import { getTimeFormatter } from '@superset-ui/time-format'; import { filterXSS } from 'xss'; @@ -202,49 +203,58 @@ export default function ReactDataTable(props: DataTableProps) { }; }); - return ( -
- - - - {columns.map(col => ( - // by default all columns will have sorting - - ))} - - - - {data.map((record, i) => ( - // hide rows after first page makes the initial render faster (less layout computation) - // eslint-disable-next-line react/no-array-index-key - 0 && i >= pageLength ? 'none' : undefined }}> - {columns.map(({ key, format }) => { - const val = record[key]; - const keyIsMetric = metricsSet.has(key); - const text = cellText(key, format, val); - const isHtml = !keyIsMetric && isProbablyHTML(text); - return ( - - ); - })} - + const tableElement = ( +
- {col.label} -
- {isHtml ? null : text} -
+ + + {columns.map(col => ( + // by default all columns will have sorting + ))} - -
+ {col.label} +
-
+ + + + {data.map((record, i) => ( + 0 && i >= pageLength ? 'none' : undefined }} + > + {columns.map(({ key, format }) => { + const val = record[key]; + const keyIsMetric = metricsSet.has(key); + const text = cellText(key, format, val); + const isHtml = !keyIsMetric && isProbablyHTML(text); + return ( + + {isHtml ? null : text} + + ); + })} + + ))} + + + ); + + return ( +
); } diff --git a/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/src/transformProps.ts b/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/src/transformProps.ts index ab26f3b3e2..a095eb4376 100644 --- a/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/src/transformProps.ts +++ b/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/src/transformProps.ts @@ -78,9 +78,11 @@ export default function transformProps(chartProps: ChartProps): DataTableProps { } = formData; const { columnFormats, verboseMap } = datasource; const { records, columns: columns_ } = queryData.data; - const metrics = metrics_.map(consolidateMetricShape); + const metrics = (metrics_ ?? []).map(consolidateMetricShape); // percent metrics always starts with a '%' sign. - const percentMetrics = percentMetrics_.map(consolidateMetricShape).map((x: string) => `%${x}`); + const percentMetrics = (percentMetrics_ ?? []) + .map(consolidateMetricShape) + .map((x: string) => `%${x}`); const columns = columns_.map((key: string) => { let label = verboseMap[key] || key; diff --git a/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/test/ReactDataTable.test.tsx b/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/test/ReactDataTable.test.tsx new file mode 100644 index 0000000000..7b7fb3b76a --- /dev/null +++ b/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/test/ReactDataTable.test.tsx @@ -0,0 +1,57 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import React from 'react'; +import { mount } from 'enzyme'; +import ReactDataTable from '../src/ReactDataTable'; +import transformProps from '../src/transformProps'; +import testData from './testData'; + +describe('legacy-table', () => { + // Can test more prop transformation here. Not needed for now. + describe('transformProps', () => {}); + + describe('ReactDataTable', () => { + let wrap: any; // the ReactDataTable wraper + + it('render basic data', () => { + wrap = mount(); + const tree = wrap.render(); // returns a CheerioWrapper with jQuery-like API + const cells = tree.find('td'); + expect(tree.hasClass('superset-legacy-chart-table')).toEqual(true); + expect(cells).toHaveLength(4); + expect(cells.eq(0).text()).toEqual('Michael'); + expect(cells.eq(3).attr('data-sort')).toEqual('2467'); + }); + + it('render advanced data', () => { + // should successfull rerender with new props + wrap.setProps(transformProps(testData.advanced)); + const tree = wrap.render(); + const cells = tree.find('td'); + expect( + tree + .find('th') + .eq(1) + .text(), + ).toEqual('Sum of Num'); + expect(cells.eq(2).text()).toEqual('12.346%'); + expect(cells.eq(4).text()).toEqual('2.47k'); + }); + }); +}); diff --git a/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/test/testData.ts b/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/test/testData.ts new file mode 100644 index 0000000000..fabb82d205 --- /dev/null +++ b/plugins/superset-ui-plugins/packages/superset-ui-legacy-plugin-chart-table/test/testData.ts @@ -0,0 +1,108 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { ChartProps } from '@superset-ui/chart'; + +const basicFormData = { + alignPn: false, + colorPn: false, + includeSearch: false, + orderDesc: true, + pageLength: 0, + metrics: [], + percentMetrics: null, + timeseriesLimitMetric: null, + tableFilter: false, + tableTimestampFormat: '%Y-%m-%d %H:%M:%S', +}; + +const basicChartProps = { + width: 200, + height: 500, + annotationData: {}, + datasource: { + columnFormats: {}, + verboseMap: {}, + }, + rawDatasource: {}, + rawFormData: {}, + hooks: {}, + initialValues: {}, + queryData: { + data: { + columns: [], + records: [], + }, + }, + formData: basicFormData, +}; + +/** + * Basic data input + */ +const basic: ChartProps = { + ...basicChartProps, + queryData: { + data: { + columns: ['name', 'sum__num'], + records: [ + { + name: 'Michael', + sum__num: 2467063, + '%pct_nice': 0.123456, + }, + { + name: 'Joe', + sum__num: 2467, + '%pct_nice': 0.00001, + }, + ], + }, + }, +}; + +/** + * Advanced data input with + * - verbose map + * - metric columns + */ +const advanced: ChartProps = { + ...basic, + datasource: { + columnFormats: {}, + verboseMap: { + sum__num: 'Sum of Num', + }, + }, + formData: { + ...basicFormData, + metrics: ['sum__num'], + percentMetrics: ['pct_nice'], + }, + queryData: { + data: { + columns: ['name', 'sum__num', '%pct_nice'], + records: [...basic.queryData.data.records], + }, + }, +}; + +export default { + basic, + advanced, +}; diff --git a/plugins/superset-ui-plugins/packages/superset-ui-plugins-demo/storybook/stories/legacy-plugin-chart-table/Stories.tsx b/plugins/superset-ui-plugins/packages/superset-ui-plugins-demo/storybook/stories/legacy-plugin-chart-table/Stories.tsx index c0869ec216..46caee75be 100644 --- a/plugins/superset-ui-plugins/packages/superset-ui-plugins-demo/storybook/stories/legacy-plugin-chart-table/Stories.tsx +++ b/plugins/superset-ui-plugins/packages/superset-ui-plugins-demo/storybook/stories/legacy-plugin-chart-table/Stories.tsx @@ -25,13 +25,32 @@ function paginated(props: SuperChartProps, pageSize = 50) { }; } +function adjustNumCols(props: SuperChartProps, numCols = 7) { + const newProps = { ...props }; + if (props.queryData) { + const { columns } = props.queryData.data; + const curSize = columns.length; + const newColumns = [...Array(numCols)].map((_, i) => { + return columns[i % curSize]; + }); + newProps.queryData = { + ...props.queryData, + data: { + ...props.queryData.data, + columns: newColumns, + }, + }; + } + return newProps; +} + /** * Load sample data for testing * @param props the original props passed to SuperChart * @param pageSize number of records perpage * @param targetSize the target total number of records */ -function loadData(props: SuperChartProps, pageSize = 50, targetSize = 10042) { +function loadData(props: SuperChartProps, pageSize = 50, targetSize = 5042) { if (!props.queryData) return props; const data = props.queryData && props.queryData.data; if (data.records.length > 0) { @@ -67,7 +86,7 @@ export default [ metrics: ['sum__num'], orderDesc: true, pageLength: 0, - percentMetrics: [], + percentMetrics: null, tableFilter: false, tableTimestampFormat: '%Y-%m-%d %H:%M:%S', timeseriesLimitMetric: null, @@ -79,27 +98,51 @@ export default [ }, { renderStory() { - const [chartProps, setChartProps] = useState(loadData(birthNames)); + const initialProps = loadData(birthNames); + const [chartProps, setChartProps] = useState(initialProps); + const updatePageSize = (size: number) => { - setChartProps(paginated(chartProps, size)); + setChartProps(paginated(initialProps, size)); + }; + const updateNumCols = (numCols: number) => { + setChartProps(adjustNumCols(initialProps, numCols)); }; + return (
- Initial page size:{' '} -
- {[10, 25, 40, 50, 100, -1].map(pageSize => { - return ( - - ); - })} +
+ Initial page size:{' '} +
+ {[10, 25, 40, 50, 100, -1].map(pageSize => { + return ( + + ); + })} +
+
+
+ Number of columns:{' '} +
+ {[1, 3, 5, 7, 9].map(numCols => { + return ( + + ); + })} +
diff --git a/plugins/superset-ui-plugins/scripts/build.js b/plugins/superset-ui-plugins/scripts/build.js index 008b75f9d0..0326d0f71c 100644 --- a/plugins/superset-ui-plugins/scripts/build.js +++ b/plugins/superset-ui-plugins/scripts/build.js @@ -4,21 +4,29 @@ const { spawnSync, spawn } = require('child_process'); const glob = process.argv[2]; +const extraArgs = process.argv.slice(2); process.env.PATH = `./node_modules/.bin:${process.env.PATH}`; -const run = (cmd, async = false) => { +const run = (cmd) => { console.log(`>> ${cmd}`); const [p, ...args] = cmd.split(' '); - const runner = async ? spawn : spawnSync; - runner(p, args, { stdio: 'inherit' }); + const runner = spawnSync; + const { status } = runner(p, args, { stdio: 'inherit' }); + if (status !== 0) { + process.exit(status); + } }; if (glob) { - run(`nimbus prettier --check --workspaces=\"@superset-ui/${glob}"`); - run(`nimbus babel --clean --workspaces=\"@superset-ui/${glob}"`); - run(`nimbus babel --clean --workspaces=\"@superset-ui/${glob}" --esm`); - run(`nimbus typescript --build --workspaces=\"@superset-ui/${glob}"`); + run(`nimbus prettier packages/${glob}/{src,test}/**/*.{js,jsx,ts,tsx,css}"`); + // lint is slow, so not turning it on by default + if (extraArgs.includes('--lint')) { + run(`nimbus eslint packages/${glob}/{src,test}`); + } + run(`nimbus babel --clean --workspaces="@superset-ui/${glob}"`); + run(`nimbus babel --clean --workspaces="@superset-ui/${glob}" --esm`); + run(`nimbus typescript --build --workspaces="@superset-ui/${glob}"`); require('./buildAssets'); } else { run('yarn build'); diff --git a/plugins/superset-ui-plugins/scripts/setupJest.js b/plugins/superset-ui-plugins/scripts/setupJest.js new file mode 100644 index 0000000000..4483e0aff4 --- /dev/null +++ b/plugins/superset-ui-plugins/scripts/setupJest.js @@ -0,0 +1,6 @@ +import { configure as configureTranslation } from '@superset-ui/translation'; +import { configure as configureEnzyme } from 'enzyme'; +import EnzymeReactAdapter from 'enzyme-adapter-react-16'; + +configureTranslation(); +configureEnzyme({ adapter: new EnzymeReactAdapter() }); diff --git a/plugins/superset-ui-plugins/yarn.lock b/plugins/superset-ui-plugins/yarn.lock index 7549bb058f..afaf7679d0 100644 --- a/plugins/superset-ui-plugins/yarn.lock +++ b/plugins/superset-ui-plugins/yarn.lock @@ -10053,6 +10053,11 @@ har-validator@~5.1.3: ajv "^6.5.5" har-schema "^2.0.0" +harmony-reflect@^1.4.6: + version "1.6.1" + resolved "https://registry.yarnpkg.com/harmony-reflect/-/harmony-reflect-1.6.1.tgz#c108d4f2bb451efef7a37861fdbdae72c9bdefa9" + integrity sha512-WJTeyp0JzGtHcuMsi7rw2VwtkvLa+JyfEKJCFyfcS0+CDkjQ5lHPu7zEhFZP+PDSRrEgXa5Ah0l1MbgbE41XjA== + has-ansi@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/has-ansi/-/has-ansi-2.0.0.tgz#34f5049ce1ecdf2b0649af3ef24e45ed35416d91" @@ -10407,6 +10412,13 @@ icss-utils@^4.0.0, icss-utils@^4.1.1: dependencies: postcss "^7.0.14" +identity-obj-proxy@^3.0.0: + version "3.0.0" + resolved "https://registry.yarnpkg.com/identity-obj-proxy/-/identity-obj-proxy-3.0.0.tgz#94d2bda96084453ef36fbc5aaec37e0f79f1fc14" + integrity sha1-lNK9qWCERT7zb7xarsN+D3nx/BQ= + dependencies: + harmony-reflect "^1.4.6" + ieee754@^1.1.12, ieee754@^1.1.4: version "1.1.13" resolved "https://registry.yarnpkg.com/ieee754/-/ieee754-1.1.13.tgz#ec168558e95aa181fd87d37f55c32bbcb6708b84"