Checkbox column appended instead of prepended in concurrent mode

[eps1lon]

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.

Expected Behavior 🤔

It should be the first column

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 (mui/material-ui#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]

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]

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

[dtassone]

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]

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]

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]

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

[eps1lon]

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]

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]

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.