search

mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.14k stars 1.29k forks source link

Checkbox column appended instead of prepended in concurrent mode #1562

Closed eps1lon closed 3 years ago

eps1lon commented 3 years ago

If you can point me to the code that's responsible for inserting the column I can take a look to understand what's going wrong if you want.

Current Behavior 😯

When using <DataGrid checkboxSelection /> the column containing the checkboxes is the last column in concurrent react.

screenshot of the current behavior

Expected Behavior 🤔

It should be the first column

screenshot of the expected behavior

Steps to Reproduce 🕹

https://codesandbox.io/s/datagrid-legacy-vs-concurrent-root-84z9o?file=/demo.js

Steps:

  1. Use react@experimental (currently 0.0.0-experimental-79740da4c)
  2. render <DataGrid rows={rows} columns={columns} pageSize={5} checkboxSelection />

Context 🔦

Reproducible in our docs: https://60906bee09db3300076bbe72--material-ui.netlify.app/components/tables/#DataTable (https://github.com/mui-org/material-ui/pull/26107)

Your Environment 🌎

See codesandbox. For the docs see the details below.

`npx @material-ui/envinfo` ``` System: OS: Linux 5.4 Ubuntu 20.04.2 LTS (Focal Fossa) Binaries: Node: 12.20.0 - ~/.nvm/versions/node/v12.20.0/bin/node Yarn: 1.22.10 - /usr/bin/yarn npm: 6.14.8 - ~/.nvm/versions/node/v12.20.0/bin/npm Browsers: Chrome: 90.0.4430.72 Firefox: 88.0 npmPackages: @emotion/react: ^11.0.0 => 11.1.5 @emotion/styled: ^11.0.0 => 11.3.0 @material-ui/codemod: 5.0.0-alpha.30 @material-ui/core: 5.0.0-alpha.32 @material-ui/data-grid: 4.0.0-alpha.27 @material-ui/docs: 5.0.0-alpha.31 @material-ui/envinfo: 1.1.6 @material-ui/icons: 5.0.0-alpha.29 @material-ui/lab: 5.0.0-alpha.32 @material-ui/private-theming: 5.0.0-alpha.32 @material-ui/styled-engine: 5.0.0-alpha.26 @material-ui/styled-engine-sc: 5.0.0-alpha.26 @material-ui/styles: 5.0.0-alpha.32 @material-ui/system: 5.0.0-alpha.31 @material-ui/types: 6.0.0 @material-ui/unstyled: 5.0.0-alpha.32 @material-ui/utils: 5.0.0-alpha.31 @types/react: ^17.0.3 => 17.0.4 react: ^17.0.1 => 0.0.0-experimental-79740da4c react-dom: ^17.0.1 => 0.0.0-experimental-79740da4c styled-components: 5.2.3 typescript: ^4.1.2 => 4.2.4 ```

Order id 💳

N/A

oliviertassinari commented 3 years ago

Oh wow, this one is amazing. I'm curious to see why. It could require changing the architecture if it surfaces a design assumption that doesn't hold.

dtassone commented 3 years ago

@eps1lon https://github.com/mui-org/material-ui-x/blob/master/packages/grid/_modules_/grid/hooks/features/columns/useGridColumns.ts#L77

dtassone commented 3 years ago

TBH, I'm not sure why we always have to question the architecture of the grid every time there is a new issue. It is probably the same kind of fix as strict mode...

Also changing the "architecture" at this stage would delay all the plans to go to beta, and stable.

Concurrent mode is in experimental stage, I don't think this issue is worth looking at this stage. It could be automatically fixed in the stable version.

oliviertassinari commented 3 years ago

In my mind, "architecture" means the main pillars we depend on to account for the constraints of the real world and to shelter the features we are building around it.

For the support of strict mode, we had to shift a bit our approach. Concurrent mode is a significant implementation shift in React, it sets new constraints. I think that the eventuality that the current structure won't hold with these new constraints is not impossible. I didn't intend to say more.

I was about to make the same point 👍. The set of constraints that concurrent mode creates is not fixed, so maybe we have an issue right now, that will go away tomorrow (the inverse is also possible).

eps1lon commented 3 years ago

Couple of things:

  1. Naming of hydrateColumns Hydration is a pretty well established term in React. However, I can't see what hydration has to do with this method. The checkbox column doesn't exist when hydrateColumns runs. So the column should probably already exists when hydrateColumns run and then you should document what is being "hydrated".
  2. Apply getDerivedStateFromProps pattern As far as I can tell you're updating state in useEffect in response to a change of props. You should apply the getDerivedStateFromProps with hooks pattern instead which gets rid of the cascading update that probably leads to the tearing we're seeing in React 18
flaviendelangle commented 3 years ago

What would be the correct naming for hydrateColumns here ? From what I understand it's a pre-processing step on the columns.

eps1lon commented 3 years ago

This is on this weeks agenda. I should have a version of this behavior without tearing in v5. If that works as I think it does then I may even be able to write a lint rule that finds these "set state state in an effect" patterns. It seems to me these patterns are just applied out of convenience so we need to find a way to make the right pattern more convenient.

eps1lon commented 3 years ago

It seems like you've implemented your own statement managment solution. I've traced the state changes and they all end up in some ref reassignment (https://github.com/mui-org/material-ui-x/blob/2cf4818737a96b983222f3c5e32d89d3303fa313/packages/grid/_modules_/grid/hooks/features/core/useGridState.ts#L22-L22) that will ultimately block most know React patterns from working.

I'm fairly certain this is just derived state but due to the custom state managment you won't be able to unlock a lot of React's own perf optimizations (especially the ones coming with concurrent rendering).

This is painfully obvious by the fact that you need to use a forceUpdate helper so often.

Consider GridComponent. The user passes in columns but then we compute the columns with the checkbox and dispatch a change event. Either the user should just pass in the columns with checkbox (optionally with a helper like <GridComponent columns={withCheckbox(columns)} /> or the checkbox should just be part of some internal state that we derive from the columns props. But dispatching a change event even though nothing actually changes is a bit odd.

But mixing these together creates problems with component composition. I've seens this mistake repeat over and over again (latest example are the picker components). And we have this pretty well documented in React in the controlled/uncontrolled components pattern and the derived state pattern.

oliviertassinari commented 3 years ago

It's behaving correctly with v4.0.0-alpha.37: https://codesandbox.io/s/datagrid-legacy-vs-concurrent-root-forked-hkp39?file=/demo.js. We changed something along the way.