From b579f3dbcb777d03176d33614a84bf8483cc7d0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andre=CC=81=20Rivet?= Date: Thu, 13 Jun 2019 19:14:59 -0400 Subject: [PATCH 1/5] isolate shared memoized calcs --- src/dash-table/dash/DataTable.js | 5 +- .../dash/{sanitize.ts => Sanitizer.ts} | 58 ++++++++++--------- 2 files changed, 35 insertions(+), 28 deletions(-) rename src/dash-table/dash/{sanitize.ts => Sanitizer.ts} (52%) diff --git a/src/dash-table/dash/DataTable.js b/src/dash-table/dash/DataTable.js index c95266b2e..8f986558b 100644 --- a/src/dash-table/dash/DataTable.js +++ b/src/dash-table/dash/DataTable.js @@ -8,13 +8,14 @@ import Logger from 'core/Logger'; import genRandomId from 'dash-table/utils/generate'; import isValidProps from './validate'; -import sanitizeProps from './sanitize'; +import Sanitizer from './Sanitizer'; export default class DataTable extends Component { constructor(props) { super(props); let id; this.getId = () => (id = id || genRandomId('table-')); + this.sanitizer = new Sanitizer(); } render() { @@ -22,7 +23,7 @@ export default class DataTable extends Component { return (
Invalid props combination
); } - const sanitizedProps = sanitizeProps(this.props); + const sanitizedProps = this.sanitizer.sanitize(this.props); return this.props.id ? () : (); diff --git a/src/dash-table/dash/sanitize.ts b/src/dash-table/dash/Sanitizer.ts similarity index 52% rename from src/dash-table/dash/sanitize.ts rename to src/dash-table/dash/Sanitizer.ts index 0e96a2444..2252cdad5 100644 --- a/src/dash-table/dash/sanitize.ts +++ b/src/dash-table/dash/Sanitizer.ts @@ -29,23 +29,6 @@ const D3_DEFAULT_LOCALE: INumberLocale = { const DEFAULT_NULLY = ''; const DEFAULT_SPECIFIER = ''; -const applyDefaultToLocale = memoizeOne((locale: INumberLocale) => getLocale(locale)); - -const applyDefaultsToColumns = memoizeOne( - (defaultLocale: INumberLocale, defaultSort: SortAsNull, columns: Columns, editable: boolean ) => R.map(column => { - const c = R.clone(column); - c.editable = isEditable(editable, column.editable); - c.sort_as_null = c.sort_as_null || defaultSort; - - if (c.type === ColumnType.Numeric && c.format) { - c.format.locale = getLocale(defaultLocale, c.format.locale); - c.format.nully = getNully(c.format.nully); - c.format.specifier = getSpecifier(c.format.specifier); - } - return c; - }, columns) -); - const data2number = (data?: any) => +data || 0; const getFixedColumns = ( @@ -64,16 +47,39 @@ const getFixedRows = ( 0 : headerRows(columns) + (filter_action !== TableAction.None ? 1 : 0) + data2number(fixed.data); -export default (props: PropsWithDefaults): SanitizedProps => { - const locale_format = applyDefaultToLocale(props.locale_format); +export default class Sanitizer { + constructor() { - return R.merge(props, { - columns: applyDefaultsToColumns(locale_format, props.sort_as_null, props.columns, props.editable), - fixed_columns: getFixedColumns(props.fixed_columns, props.row_deletable, props.row_selectable), - fixed_rows: getFixedRows(props.fixed_rows, props.columns, props.filter_action), - locale_format - }); -}; + } + + sanitize(props: PropsWithDefaults): SanitizedProps { + const locale_format = this.applyDefaultToLocale(props.locale_format); + + return R.merge(props, { + columns: this.applyDefaultsToColumns(locale_format, props.sort_as_null, props.columns, props.editable), + fixed_columns: getFixedColumns(props.fixed_columns, props.row_deletable, props.row_selectable), + fixed_rows: getFixedRows(props.fixed_rows, props.columns, props.filter_action), + locale_format + }); + } + + private readonly applyDefaultToLocale = memoizeOne((locale: INumberLocale) => getLocale(locale)); + + private readonly applyDefaultsToColumns = memoizeOne( + (defaultLocale: INumberLocale, defaultSort: SortAsNull, columns: Columns, editable: boolean) => R.map(column => { + const c = R.clone(column); + c.editable = isEditable(editable, column.editable); + c.sort_as_null = c.sort_as_null || defaultSort; + + if (c.type === ColumnType.Numeric && c.format) { + c.format.locale = getLocale(defaultLocale, c.format.locale); + c.format.nully = getNully(c.format.nully); + c.format.specifier = getSpecifier(c.format.specifier); + } + return c; + }, columns) + ); +} export const getLocale = (...locales: Partial[]): INumberLocale => R.mergeAll([ From 46b8824ebfe25dcad5824a0fc500bb0ded1edad0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andre=CC=81=20Rivet?= Date: Fri, 14 Jun 2019 14:32:32 -0400 Subject: [PATCH 2/5] - add cell edi test with two tables - revert sanitizer to failing impl - app.js -> tsx --- demo/App.js | 75 ----------------- demo/App.tsx | 84 +++++++++++++++++++ demo/AppMode.ts | 2 + src/dash-table/dash/Sanitizer.ts | 40 +++++---- .../tests/standalone/edit_cell_test.ts | 42 ++++++---- 5 files changed, 133 insertions(+), 110 deletions(-) delete mode 100644 demo/App.js create mode 100644 demo/App.tsx diff --git a/demo/App.js b/demo/App.js deleted file mode 100644 index 6c189180d..000000000 --- a/demo/App.js +++ /dev/null @@ -1,75 +0,0 @@ -/* eslint no-magic-numbers: 0 */ -import * as R from 'ramda'; -import React, {Component} from 'react'; -import { DataTable } from 'dash-table'; -import Environment from 'core/environment'; -import { memoizeOne } from 'core/memoizer'; -import Logger from 'core/Logger'; -import AppState, { AppMode } from './AppMode'; - -import './style.less'; - -class App extends Component { - constructor() { - super(); - - this.state = AppState; - this.state.temp_filtering = ''; - - const setProps = memoizeOne(() => { - return newProps => { - Logger.debug('--->', newProps); - this.setState(prevState => ({ - tableProps: R.merge(prevState.tableProps, newProps) - })); - }; - }); - - Object.defineProperty(this, 'setProps', { - get: () => setProps() - }); - } - - renderMode() { - const mode = Environment.searchParams.get('mode'); - - if (mode === AppMode.Filtering) { - return ( -
- - this.setState({ temp_filtering: e.target.value }) - } - onBlur={e => { - const tableProps = R.clone(this.state.tableProps); - tableProps.filter_query = e.target.value; - - this.setState({ tableProps }); - }} /> -
); - } - } - - render() { - return (
- {this.renderMode()} - -
); - } -} - -export default App; diff --git a/demo/App.tsx b/demo/App.tsx new file mode 100644 index 000000000..160d0a341 --- /dev/null +++ b/demo/App.tsx @@ -0,0 +1,84 @@ +/* eslint no-magic-numbers: 0 */ +import * as R from 'ramda'; +import React, {Component} from 'react'; +import { DataTable } from 'dash-table/index'; +import Environment from 'core/environment'; +import { memoizeOne } from 'core/memoizer'; +import Logger from 'core/Logger'; +import AppState, { AppMode } from './AppMode'; +import memoizerCache from 'core/cache/memoizer'; + +import './style.less'; + +class App extends Component { + constructor(props: any) { + super(props); + + this.state = { + ...AppState, + temp_filtering: '' + }; + } + + renderMode() { + const mode = Environment.searchParams.get('mode'); + + if (mode === AppMode.Filtering) { + return (
+ + this.setState({ temp_filtering: e.target.value }) + } + onBlur={e => { + const tableProps = R.clone(this.state.tableProps); + tableProps.filter_query = e.target.value; + + this.setState({ tableProps }); + }} /> +
); + } else if (mode === AppMode.TaleOfTwoTables) { + const props: any = {}; + Object.entries(this.state.tableProps).forEach(([key, value]) => { + props[key] = this.propCache.get(key)(value); + }); + + return (); + } + } + + render() { + return (
+ {this.renderMode()} + +
); + } + + private propCache = memoizerCache<[string]>()(R.clone); + + private setProps = memoizeOne(() => { + return (newProps: any) => { + Logger.debug('--->', newProps); + this.setState((prevState: any) => ({ + tableProps: R.merge(prevState.tableProps, newProps) + })); + }; + }); +} + +export default App; diff --git a/demo/AppMode.ts b/demo/AppMode.ts index 97aec6b10..06f3fa470 100644 --- a/demo/AppMode.ts +++ b/demo/AppMode.ts @@ -22,6 +22,7 @@ export enum AppMode { Formatting = 'formatting', ReadOnly = 'readonly', ColumnsInSpace = 'columnsInSpace', + TaleOfTwoTables = 'taleOfTwoTables', Tooltips = 'tooltips', Typed = 'typed', Virtualized = 'virtualized' @@ -335,6 +336,7 @@ function getState() { return getVirtualizedState(); case AppMode.Typed: return getTypedState(); + case AppMode.TaleOfTwoTables: case AppMode.Default: default: return getDefaultState(); diff --git a/src/dash-table/dash/Sanitizer.ts b/src/dash-table/dash/Sanitizer.ts index 2252cdad5..ecfe78d39 100644 --- a/src/dash-table/dash/Sanitizer.ts +++ b/src/dash-table/dash/Sanitizer.ts @@ -47,38 +47,36 @@ const getFixedRows = ( 0 : headerRows(columns) + (filter_action !== TableAction.None ? 1 : 0) + data2number(fixed.data); -export default class Sanitizer { - constructor() { - +const applyDefaultsToColumns = (defaultLocale: INumberLocale, defaultSort: SortAsNull, columns: Columns, editable: boolean) => R.map(column => { + const c = R.clone(column); + c.editable = isEditable(editable, column.editable); + c.sort_as_null = c.sort_as_null || defaultSort; + + if (c.type === ColumnType.Numeric && c.format) { + c.format.locale = getLocale(defaultLocale, c.format.locale); + c.format.nully = getNully(c.format.nully); + c.format.specifier = getSpecifier(c.format.specifier); } + return c; +}, columns); + +const applyDefaultToLocale = (locale: INumberLocale) => getLocale(locale); +export default class Sanitizer { sanitize(props: PropsWithDefaults): SanitizedProps { - const locale_format = this.applyDefaultToLocale(props.locale_format); + const locale_format = Sanitizer.applyDefaultToLocale(props.locale_format); return R.merge(props, { - columns: this.applyDefaultsToColumns(locale_format, props.sort_as_null, props.columns, props.editable), + columns: Sanitizer.applyDefaultsToColumns(locale_format, props.sort_as_null, props.columns, props.editable), fixed_columns: getFixedColumns(props.fixed_columns, props.row_deletable, props.row_selectable), fixed_rows: getFixedRows(props.fixed_rows, props.columns, props.filter_action), locale_format }); } - private readonly applyDefaultToLocale = memoizeOne((locale: INumberLocale) => getLocale(locale)); - - private readonly applyDefaultsToColumns = memoizeOne( - (defaultLocale: INumberLocale, defaultSort: SortAsNull, columns: Columns, editable: boolean) => R.map(column => { - const c = R.clone(column); - c.editable = isEditable(editable, column.editable); - c.sort_as_null = c.sort_as_null || defaultSort; - - if (c.type === ColumnType.Numeric && c.format) { - c.format.locale = getLocale(defaultLocale, c.format.locale); - c.format.nully = getNully(c.format.nully); - c.format.specifier = getSpecifier(c.format.specifier); - } - return c; - }, columns) - ); + private static readonly applyDefaultToLocale = memoizeOne(applyDefaultToLocale); + + private static readonly applyDefaultsToColumns = memoizeOne(applyDefaultsToColumns); } export const getLocale = (...locales: Partial[]): INumberLocale => diff --git a/tests/cypress/tests/standalone/edit_cell_test.ts b/tests/cypress/tests/standalone/edit_cell_test.ts index 152063088..0ad1ca45a 100644 --- a/tests/cypress/tests/standalone/edit_cell_test.ts +++ b/tests/cypress/tests/standalone/edit_cell_test.ts @@ -4,6 +4,34 @@ import Key from 'cypress/Key'; import { AppMode, ReadWriteModes } from 'demo/AppMode'; +Object.values([ + ...ReadWriteModes, + AppMode.TaleOfTwoTables +]).forEach(mode => { + describe(`edit, mode=${mode}`, () => { + beforeEach(() => { + cy.visit(`http://localhost:8080?mode=${mode}`); + DashTable.toggleScroll(false); + }); + + // https://github.com/plotly/dash-table/issues/50 + it('can edit on "enter"', () => { + DashTable.getCell(0, 1).click(); + cy.wait(1000); + DOM.focused.type(`abc${Key.Enter}`); + DashTable.getCell(0, 1).within(() => cy.get('.dash-cell-value').should('have.html', `abc`)); + }); + + it('can edit when clicking outside of cell', () => { + DashTable.getCell(0, 1).click(); + DOM.focused.type(`abc`); + cy.wait(1000); + DashTable.getCell(0, 0).click(); + DashTable.getCell(0, 1).within(() => cy.get('.dash-cell-value').should('have.html', `abc`)); + }); + }); +}); + Object.values(ReadWriteModes).forEach(mode => { describe(`edit, mode=${mode}`, () => { beforeEach(() => { @@ -106,20 +134,6 @@ Object.values(ReadWriteModes).forEach(mode => { cy.get('.Select-value-label').should('have.html', expectedValue); }); }); - - // https://github.com/plotly/dash-table/issues/50 - it('can edit on "enter"', () => { - DashTable.getCell(0, 1).click(); - DOM.focused.type(`abc${Key.Enter}`); - DashTable.getCell(0, 1).within(() => cy.get('.dash-cell-value').should('have.html', `abc`)); - }); - - it('can edit when clicking outside of cell', () => { - DashTable.getCell(0, 1).click(); - DOM.focused.type(`abc`); - DashTable.getCell(0, 0).click(); - DashTable.getCell(0, 1).within(() => cy.get('.dash-cell-value').should('have.html', `abc`)); - }); }); }); From c28bf8149740b8dcc22cfe76351d5179e6b31298 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andre=CC=81=20Rivet?= Date: Fri, 14 Jun 2019 14:42:57 -0400 Subject: [PATCH 3/5] update test --- tests/cypress/tests/unit/formatting_test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/cypress/tests/unit/formatting_test.ts b/tests/cypress/tests/unit/formatting_test.ts index 3e76d6fc0..e0262fd9b 100644 --- a/tests/cypress/tests/unit/formatting_test.ts +++ b/tests/cypress/tests/unit/formatting_test.ts @@ -1,5 +1,5 @@ import { getFormatter } from 'dash-table/type/number'; -import { getLocale, getNully, getSpecifier } from 'dash-table/dash/sanitize'; +import { getLocale, getNully, getSpecifier } from 'dash-table/dash/Sanitizer'; describe('formatting', () => { describe('number', () => { From 4db77d184cdb21883a5e139e41ff77935522fa7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andre=CC=81=20Rivet?= Date: Fri, 14 Jun 2019 14:45:10 -0400 Subject: [PATCH 4/5] fix sanitation memoize --- src/dash-table/dash/Sanitizer.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/dash-table/dash/Sanitizer.ts b/src/dash-table/dash/Sanitizer.ts index ecfe78d39..ea35b471d 100644 --- a/src/dash-table/dash/Sanitizer.ts +++ b/src/dash-table/dash/Sanitizer.ts @@ -64,19 +64,19 @@ const applyDefaultToLocale = (locale: INumberLocale) => getLocale(locale); export default class Sanitizer { sanitize(props: PropsWithDefaults): SanitizedProps { - const locale_format = Sanitizer.applyDefaultToLocale(props.locale_format); + const locale_format = this.applyDefaultToLocale(props.locale_format); return R.merge(props, { - columns: Sanitizer.applyDefaultsToColumns(locale_format, props.sort_as_null, props.columns, props.editable), + columns: this.applyDefaultsToColumns(locale_format, props.sort_as_null, props.columns, props.editable), fixed_columns: getFixedColumns(props.fixed_columns, props.row_deletable, props.row_selectable), fixed_rows: getFixedRows(props.fixed_rows, props.columns, props.filter_action), locale_format }); } - private static readonly applyDefaultToLocale = memoizeOne(applyDefaultToLocale); + private readonly applyDefaultToLocale = memoizeOne(applyDefaultToLocale); - private static readonly applyDefaultsToColumns = memoizeOne(applyDefaultsToColumns); + private readonly applyDefaultsToColumns = memoizeOne(applyDefaultsToColumns); } export const getLocale = (...locales: Partial[]): INumberLocale => From 87e3f3865a956cc162b7fa63580754fe7794fea7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andre=CC=81=20Rivet?= Date: Mon, 17 Jun 2019 09:20:47 -0400 Subject: [PATCH 5/5] add explanation for weird test setup --- tests/cypress/tests/standalone/edit_cell_test.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/cypress/tests/standalone/edit_cell_test.ts b/tests/cypress/tests/standalone/edit_cell_test.ts index 0ad1ca45a..7f298f452 100644 --- a/tests/cypress/tests/standalone/edit_cell_test.ts +++ b/tests/cypress/tests/standalone/edit_cell_test.ts @@ -14,9 +14,14 @@ Object.values([ DashTable.toggleScroll(false); }); - // https://github.com/plotly/dash-table/issues/50 + // Case: "Pressing enter to confirm change does not work on the last row" + // Issue: https://github.com/plotly/dash-table/issues/50 it('can edit on "enter"', () => { DashTable.getCell(0, 1).click(); + // Case: 2+ tables results in infinite rendering loop b/c of shared cache + // Issue: https://github.com/plotly/dash-table/pull/468 + // + // Artificial delay added to make sure re-rendering has time to occur. cy.wait(1000); DOM.focused.type(`abc${Key.Enter}`); DashTable.getCell(0, 1).within(() => cy.get('.dash-cell-value').should('have.html', `abc`)); @@ -25,6 +30,10 @@ Object.values([ it('can edit when clicking outside of cell', () => { DashTable.getCell(0, 1).click(); DOM.focused.type(`abc`); + // Case: 2+ tables results in infinite rendering loop b/c of shared cache + // Issue: https://github.com/plotly/dash-table/pull/468 + // + // Artificial delay added to make sure re-rendering has time to occur. cy.wait(1000); DashTable.getCell(0, 0).click(); DashTable.getCell(0, 1).within(() => cy.get('.dash-cell-value').should('have.html', `abc`));