Regression fix multi table typing#468
Conversation
- revert sanitizer to failing impl - app.js -> tsx
|
|
||
| return (<DataTable | ||
| {...props} | ||
| />); |
There was a problem hiding this comment.
Two tables mode: shallow copy each prop & memoize -- feed the 2nd table with these new values.
|
|
||
| import './style.less'; | ||
|
|
||
| class App extends Component<any, any> { |
There was a problem hiding this comment.
Renaming to .tsx file but mostly just filling in with any. Can improve some more later. At least now it's TS too.
| } | ||
|
|
||
| const sanitizedProps = sanitizeProps(this.props); | ||
| const sanitizedProps = this.sanitizer.sanitize(this.props); |
There was a problem hiding this comment.
Each table instance has its sanitizer instance
|
|
||
| private static readonly applyDefaultToLocale = memoizeOne(applyDefaultToLocale); | ||
|
|
||
| private static readonly applyDefaultsToColumns = memoizeOne(applyDefaultsToColumns); |
There was a problem hiding this comment.
static readonly on purpose to see the tests fail for this mode. This is essentially equivalent to the previous implementation. Will make readonly once a test run fails to demonstrate the impact.
| }); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Subset of edit tests that are known to be impacted by the presence of a second table.
Pulled out from the tests below and applied to one more flavor.
| it('can edit when clicking outside of cell', () => { | ||
| DashTable.getCell(0, 1).click(); | ||
| DOM.focused.type(`abc`); | ||
| cy.wait(1000); |
There was a problem hiding this comment.
The delay is not great but need to guarantee the table has had time to re-render if stuck in a loop.
There was a problem hiding this comment.
Hmm OK. I guess if it's an async system causing the infinite loop that makes sense. Please put a comment by each of these waits referencing this PR so we know why it's there and we don't try to optimize it away later, since presumably the test will still pass if it were removed - it just wouldn't be testing what it's supposed to be testing.
alexcjohnson
left a comment
alexcjohnson
left a comment
There was a problem hiding this comment.
Fantastic detective work. Just one little request for comments then this is good to go! 💃
…/dash-table into regression-fix-multi-table-typing
3 participants
#446 introduced a regression in the table sanitation logic causing a memoized functions to be shared across tables, resulting in an infinite loop re-rendering of all tables involve.
edittests for the new 'two tables' scenario (w/o fix: https://circleci.com/gh/plotly/dash-table/9324, w/ fix: https://circleci.com/gh/plotly/dash-table/9333