[DataGrid] Refactor state #412

dtassone · 2020-10-07T18:12:39Z

No description provided.

oliviertassinari

We are getting closer. I think that we should address the following before moving forward (so we keep master in a state that can be released at any time). Maybe @DanailH could help? :)

🚀 The documentation is unusable. There is a major performance regression on https://deploy-preview-412--material-ui-x.netlify.app/components/data-grid/. You can check this performance test, it becomes like the storybook (take about 8s to be ready).
📚 Could you add a description to the pull request? This would help to explain the issue, the solution, the options that were considered. If we do it orally, we will cover the problem on the surface. We won't also be able to go back to it in x months. Or even link to new developers joining the team. I think that the bigger the pull request is, the more relevant this feedback is.
🐛 The resizing feature is no longer working.

🚀 In the sorting feature, restoring the default sort order doesn't longer feel instant. It goes from 50ms to 500ms (on my machine)
⚛️ I think that changes in end-to-ends tests for refactorization are always something to be extremely cautious with. If the tests are well written and if the refacto implements the same behavior, they shouldn't be any changes. I have seen changes in the test that suggests regressions (skip, sleep, tests no longer act-synchronous).
⚛️ Some parts of the grid reducer logic seem not idiomatic to React, it might cause bugs. See the second review to have more info.
🚀 Click the select all rows freeze the whole UI for ~5s
🐛 The keyboard logic is broken. When you use it with two data tables on the same page, it jumps between the two. The focus leaks
🐛 Once you start using the keyboard logic, the virtualization becomes unresponsive. There are major white areas.
🐛 The keyboard logic is unresponsive, it takes 250ms easily to handle a press. It seems to be an infinite ref loop issue.

I have run all my tests with the main demo.
I'm having a look at what's going on with 1. 5. 6. and 7, see if I can help.

and 7. handled in Performance issue dtassone/material-ui-x#3

oliviertassinari · 2020-10-25T15:15:05Z

packages/grid/_modules_/grid/GridComponent.tsx

-      internalOptions.autoHeight,
-      internalOptions.headerHeight,
-      internalOptions.rowHeight,
+import useForkRef from '@material-ui/core/utils/useForkRef';


This import is invalid. It leads to bundling duplicate versions of the same modules (CJS + ESM, which harm bundle size).

Suggested change

import useForkRef from '@material-ui/core/utils/useForkRef';

import { useForkRef } from '@material-ui/core/utils';

We will get an error once we enable the eslint rule back:

https://github.com/mui-org/material-ui-x/blob/f7856e13759740fd21df73e1f5f3902fd1d08444/.eslintrc.js#L24-L25

We also have this issue in the main repo, mui/material-ui#23159 expose some cases.

oliviertassinari · 2020-10-25T15:16:53Z

packages/demo-app/src/app/app.tsx

@@ -1,7 +1,6 @@
 import * as React from 'react';
 import { HashRouter, Route, Switch } from 'react-router-dom';
 import styled, { createGlobalStyle } from 'styled-components';
-import './app.less';


For any large pull requests. I think that we should strongly encourage to move any change that can be prune outside of it. For instance here, we go after fixing the build of the demo-app. We can deliver value by fixing it independently from the refactoring. Then, we can rebase this pull request on the fix once it lands in master.

oliviertassinari · 2020-10-25T15:24:07Z

packages/grid/_modules_/grid/components/cell.tsx

+  const cellRef = React.useRef<HTMLDivElement>(null);
+
+  React.useEffect(() => {
+    if (hasFocus && cellRef.current) {


cellRef.current should always be defined, by contract with React.

Suggested change

if (hasFocus && cellRef.current) {

if (hasFocus) {

if it's not, something is wrong in the core structure, better not hiding it.

the initial value is null, and it gets defined after the component mount. So I can remove it and add a ! which won't change much.

! save bundle size but more importantly it allows us to not silently ignore the problem if, in a future refactorization, we forget to apply the ref correctly.

oliviertassinari · 2020-10-25T15:25:41Z

packages/grid/_modules_/grid/components/column-headers.tsx


  return <React.Fragment>{items}</React.Fragment>;
-});
+};
+ColumnHeaderItemCollection.displayName = 'ColumnHeaderItemCollection';


It has no impact, the component isn't memo or forward ref, if we do, better name the function to get bundle size once it lands in production.

Suggested change

ColumnHeaderItemCollection.displayName = 'ColumnHeaderItemCollection';

oliviertassinari · 2020-10-25T15:27:55Z

packages/grid/_modules_/grid/components/row-cells.tsx

    row,
    rowIndex,
    scrollSize,
+    cellFocus,
+    showCellRightBorder,


Best to keep the props sort ASC, more structure make it simpler to navigate it.

oliviertassinari · 2020-10-25T16:04:46Z

packages/grid/tsconfig.build.json

@@ -5,6 +5,7 @@
    "declarationDir": "./dist",
    "module": "es6",
    "noImplicitAny": false,
+    "noUnusedLocals": false,


Suggested change

"noUnusedLocals": false,

Note that I normalize the tsconfig in #448.

oliviertassinari · 2020-10-25T16:06:13Z

packages/grid/yarn.lock

+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
+# yarn lockfile v1
+
+
+reselect@^4.0.0:
+  version "4.0.0"
+  resolved "https://registry.yarnpkg.com/reselect/-/reselect-4.0.0.tgz#f2529830e5d3d0e021408b246a206ef4ea4437f7"
+  integrity sha512-qUgANli03jjAyGlnbYVAV5vvnOmJnODyABz51RdBN7M4WaVu8mecZWgyQNkG8Yqe3KRGRt0l4K4B3XVEULC4CA==


Suggested change

# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

# yarn lockfile v1

reselect@^4.0.0:

version "4.0.0"

resolved "https://registry.yarnpkg.com/reselect/-/reselect-4.0.0.tgz#f2529830e5d3d0e021408b246a206ef4ea4437f7"

integrity sha512-qUgANli03jjAyGlnbYVAV5vvnOmJnODyABz51RdBN7M4WaVu8mecZWgyQNkG8Yqe3KRGRt0l4K4B3XVEULC4CA==

oliviertassinari · 2020-10-25T16:06:46Z

packages/storybook/tsconfig.json

@@ -17,7 +17,7 @@
    "noImplicitThis": true,
    "noImplicitAny": false,
    "strictNullChecks": true,
-    "noUnusedLocals": true,
+    "noUnusedLocals": false,


Suggested change

"noUnusedLocals": false,

"noUnusedLocals": true,

tsconfig.json

oliviertassinari · 2020-10-25T16:36:08Z

packages/grid/data-grid/src/DataGrid.test.tsx

+    // eslint-disable-next-line mocha/no-skipped-tests
+    xit('should throw if the rows has no id', function test() {


Suggested change

// eslint-disable-next-line mocha/no-skipped-tests

xit('should throw if the rows has no id', function test() {

it('should throw if the rows has no id', function test() {

oliviertassinari · 2020-10-25T17:55:30Z

and 7. handled in Performance issue dtassone/material-ui-x#3

oliviertassinari

🐛 A new bug I have seen: If you add a onChange callback, it's called infinitely. For instance:

      <DataGrid
        page={page}
        onPageChange={(params) => {
          console.log('onchange')
          setPage(params.page);
        }}
        pageSize={5}
        pagination
        {...data}
      />

I had a closer look at 6. I have left a few extra comments where relevant :).

I don't understand how the new state structure solves "the problem". From what I understand, we are missing two important parts of the equation:

Updates across hook and components. How is useGridReducer different from React.useReducer? It seems to behave identically (at the exception that useGridReducer has a global imperative side effect).
Here is an example of what I mean by "the problem". The useColumnReorder() needs to set in the state that the column field id Y is reordering. How will this notification ends re-rendering <ColumnHeaderItem> (and only) so it can add the correct CSS class name? I see that we have an useSelector equivalent API to redux: useGridReducer but it's not wired. Shouldn't we listen to the event emitter here? I assume the memo we have in place will prune the rendering.
The public API. How does it fit into [RFC] Change API state #190? For instance, controlled pagination. Forcing: <DataGrid page={1}> is not working, the page can be changed, from the internal state. It's not frozen on 1 as a developer would expect. The prop behaves like a default prop. A solution would probably start with something in this order:

diff --git a/packages/grid/_modules_/grid/hooks/features/pagination/usePagination.ts b/packages/grid/_modules_/grid/hooks/features/pagination/usePagination.ts
index 6af6f657..549697c4 100644
--- a/packages/grid/_modules_/grid/hooks/features/pagination/usePagination.ts
+++ b/packages/grid/_modules_/grid/hooks/features/pagination/usePagination.ts
@@ -99,9 +99,9 @@ export const usePagination = (apiRef: ApiRef): PaginationProps => {
     dispatch(setPaginationModeActionCreator({ paginationMode: options.paginationMode! }));
   }, [apiRef, dispatch, options.paginationMode]);

-  React.useEffect(() => {
-    setPage(options.page != null ? options.page : 1);
-  }, [options.page, setPage]);
+  if (options.page != null) {
+    setPage(options.page);
+  }

   React.useEffect(() => {
     if (!options.autoPageSize && options.pageSize) {

For the uncontrolled mode. Will we have a global defaultState prop? Will we allow to reduce all the events with a stateReducer prop?
For the controlled mode. Will we allow developer to force a state prop? Will we add a onStateChange prop?

packages/grid/_modules_/grid/hooks/features/pagination/usePagination.ts

packages/grid/_modules_/grid/hooks/features/core/useGridReducer.ts

packages/grid/_modules_/grid/hooks/features/core/useGridState.ts

oliviertassinari · 2020-10-25T18:53:31Z

packages/grid/_modules_/grid/hooks/features/core/useGridSelector.ts

+  apiRef: ApiRef | undefined,
+  selector: (state: any) => State,
+) => {
+  const [state, ,] = useGridState(apiRef!);


Suggested change

const [state, ,] = useGridState(apiRef!);

const [state] = useGridState(apiRef!);

Refactored state to attach it to apiRef. Performance issues will be treated in following PRs * WIP state refactoring on pagination * fix merging issues * WIP working on the rowsReducer and global state * fixes state refactoring finalizing api * refactored grid reducer * fix issue with state and cleanup * fix state and added sortedSelctor for viewport * prettier * fix state virtualisation * small refactoring with selector * rollback lint rules * remove options prop to useGridSelector * prettier * cleanup commented code * fix pagination and rendering issues * fix sorting and refactored hooks props * Refactored useSorting to new gridState * rebased and fix issues * cleanup * prettier * fix test * rebased * fix demo app * small refactoring on header item * fix dependency cycle * fix index * prettier * removed unused fn * fixing test * small refactoringg * prettier * perf improvements and keyboard refactoring * improve perf and fix keyboard and test * move virtualisation folder * prettier * refactor useComponents and useEvents * yarn docs ts formatted * fix demos and issue with resizing * fix types in doc * small fix prettier * fix rows perf, and prettier * fix dependency array * fix prettier * restored rowIndex

dtassone requested review from oliviertassinari and DanailH October 15, 2020 15:46

dtassone force-pushed the justGridState branch from 655ceba to ad12572 Compare October 20, 2020 16:22

dtassone added 22 commits October 21, 2020 15:47

WIP state refactoring on pagination

af1df68

fix merging issues

9f89b01

WIP working on the rowsReducer and global state

95434d5

fixes state refactoring finalizing api

153813f

refactored grid reducer

a2ac04b

fix issue with state and cleanup

3cbdb8d

fix state and added sortedSelctor for viewport

6d26841

prettier

aabf153

fix state virtualisation

853198e

small refactoring with selector

84e77d5

rollback lint rules

d874bae

remove options prop to useGridSelector

5d3d8e3

prettier

31c2a96

cleanup commented code

5220264

fix pagination and rendering issues

9359199

fix sorting and refactored hooks props

55d6d99

Refactored useSorting to new gridState

ed6a93e

rebased and fix issues

a6c9bf2

cleanup

ed67afe

prettier

e3d03a3

fix test

2817bf9

rebased

d847d5f

dtassone force-pushed the justGridState branch from da62f82 to d847d5f Compare October 21, 2020 14:05

dtassone added 4 commits October 21, 2020 16:38

fix demo app

ae53a83

small refactoring on header item

5b232d4

fix dependency cycle

091a07f

fix index

19c4ede

dtassone added 5 commits October 22, 2020 12:13

prettier

9bb2d3b

perf improvements and keyboard refactoring

caa1398

improve perf and fix keyboard and test

82ec871

move virtualisation folder

77bcac0

prettier

2f4586a

oliviertassinari changed the title ~~Just grid state~~ [DataGrid] Refactor state Oct 25, 2020

oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes labels Oct 25, 2020

oliviertassinari requested changes Oct 25, 2020

View reviewed changes

oliviertassinari reviewed Oct 25, 2020

View reviewed changes

dtassone added 9 commits October 26, 2020 12:44

refactor useComponents and useEvents

a4c6e47

yarn docs ts formatted

4de3fd9

fix demos and issue with resizing

37e2f19

fix types in doc

75ce9fc

small fix prettier

20e4a54

fix rows perf, and prettier

30e7853

fix dependency array

e0801e3

fix prettier

bc93224

restored rowIndex

7466c49

dtassone merged commit 2abef4a into mui:master Oct 26, 2020

oliviertassinari mentioned this pull request Oct 26, 2020

[DataGrid] Restore regression test #503

Merged

oliviertassinari added a commit to oliviertassinari/mui-x that referenced this pull request Oct 31, 2020

[DataGrid] Apply review from mui#412

cea2e62

oliviertassinari mentioned this pull request Oct 31, 2020

[DataGrid] Apply review from #412 #515

Merged

oliviertassinari added a commit to oliviertassinari/mui-x that referenced this pull request Nov 4, 2020

[DataGrid] Apply review from mui#412

718f943

oliviertassinari added a commit that referenced this pull request Nov 4, 2020

[DataGrid] Apply review from #412 (#515)

069daa7

dtassone pushed a commit to dtassone/material-ui-x that referenced this pull request Nov 9, 2020

[DataGrid] Apply review from mui#412 (mui#515)

c0e5fa5

oliviertassinari mentioned this pull request Nov 10, 2020

[DataGrid] server side pagination not working #568

Closed

2 tasks

oliviertassinari mentioned this pull request Apr 11, 2021

[DataGrid] Refactor useGridColumnResize #1380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Refactor state #412

[DataGrid] Refactor state #412

dtassone commented Oct 7, 2020 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari Oct 25, 2020 •

edited

Loading

oliviertassinari Oct 25, 2020

oliviertassinari Oct 25, 2020

dtassone Oct 26, 2020

oliviertassinari Oct 26, 2020

oliviertassinari Oct 25, 2020

oliviertassinari Oct 25, 2020

oliviertassinari Oct 25, 2020

oliviertassinari Oct 25, 2020

oliviertassinari Oct 25, 2020

oliviertassinari Oct 25, 2020

oliviertassinari commented Oct 25, 2020

oliviertassinari left a comment

oliviertassinari Oct 25, 2020

	import useForkRef from '@material-ui/core/utils/useForkRef';
	import { useForkRef } from '@material-ui/core/utils';

		// eslint-disable-next-line mocha/no-skipped-tests
		xit('should throw if the rows has no id', function test() {

	// eslint-disable-next-line mocha/no-skipped-tests
	xit('should throw if the rows has no id', function test() {
	it('should throw if the rows has no id', function test() {

	const [state, ,] = useGridState(apiRef!);
	const [state] = useGridState(apiRef!);

[DataGrid] Refactor state #412

[DataGrid] Refactor state #412

Conversation

dtassone commented Oct 7, 2020 • edited Loading

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

oliviertassinari Oct 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Oct 25, 2020

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtassone commented Oct 7, 2020 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari Oct 25, 2020 •

edited

Loading