[DataGrid] Potential fix for grid row state propagation

[lauri865]

• I have followed (at least) the PR section of the contributing guide.

After some further thoughts on #15616, probably better to try and fix the root cause of the row existence checks instead.

@romgrk, what if anything am I breaking here? Fixes the need for try/catch and out of order state propagation (currently on removing a row, first all the cells re-render to null, then the rows unmount. After this change, a row would be responsible for cells being rendered or not).

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[mui-bot]

Deploy preview: https://deploy-preview-15627--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 54f4a1b

[lauri865]

All tests should pass now. To me this approach makes much more sense:

• Baseline row state is owned and provided by rows that render the cells.
• We sprinkle in reactivity to cells based on more atomic selectors, which makes state flow more understandable and debuggable. Currently, all the valueGetters, formatters, etc. have to re-evaluate on each and every state change, which is wasteful.

I was slightly confused why the master detail test started failing, but after looking into it, I'm more worried that it worked the way it did – a breeding ground for race conditions.

The only reason I can think of why the other way around may have some merit is if the cells would be atomically updated based on changing row values. But since all the APIs expose the row to all the cells anyways, that cannot be the case.

Also squeezed in a tiny change that fixes every row selection checkbox re-rendering on every selection model change. Was such a tiny change that I didn't bother making it a new.

[romgrk]

Is moving this block required?

I prefer to keep functional components organized as if it were a class component:

class GridRow {
  constructor() { ... }
  onClick() { ... }
  getCell() { ... }
  render() {}
}

function GridRow {
  /* initialization code */
  const onClick = () => { ... }
  const getCell = () => { ... }
  /* render code */
}

In particular for large components, it helps readability a lot to keep all render code grouped rather than spread in-between "class methods".

[lauri865]

I like grouping logic, but here the balance is between a pretty large dead code path (defining an anyonymous function that should never be called, unless rowNode exists) vs. grouping logic. Not that clear what's more readable.

After this change, if you were to accidentally remove the rowNode check on component-level, the types would break (good imho), which is better than adding an extra conditional in the getCell that would open up the potential for using the compnent in currently unintended/undesigned ways.

Probably the right question here to ask is why the component needs to (try to) mount without a rowNode to begin with.

[romgrk]

Haven't looked in details, but at first glance this looks wrong to me. This function does state reads, and those should always be wrapped in useGridSelector to enable reactive updates when the state changes. Is there a reason for which this doesn't need to be wrapped? If yes, then we probably need a comment to explain why we don't wrap with useGridSelector.

[lauri865]

That's actually one of the core changes that is a step in the right direction imho, tried to explain it briefly here: #15627 (comment)

The current behaviour casts an incredibly wide net to cell-level reactivity that opens it up to race conditions, and prevents from carefully managing state propagation.

Should cell valueGetter and valueFormatter of each cell in the datagrid (renderecontext) re-evaluate whenever any part of grid state changes? (focus, tabindex, rendercontext, etc.?). They currently do, but I would argue they shouldn't. They should only be re-evaluated if the row changes (through edits or row updates).

Imagine if you define a valueFormatter that formats the cell value based on external state. Changing that external state wouldn't update the rendered cell value, but changing the focus or scrolling a bit would suddenly trigger the cell to re-render with a new value. What?!

Want to understand why the cell re-rendered? Most of the cases, the answer is that getCellParams changed. But why did they change? Good luck understanding that.

So, ideally:

• Row data is provided by row (or editing api)
• Rest of the conditions why the cell should re-render independent of the row should be carefully managed based on atomic selectors. As you can see, the scope of selectors that should trigger cell-level reactivity currently is not that wide in order to make the tests pass. This should result in both more optimised code as well as more understandable state flow within the grid.

[romgrk]

I understand why we're adding those params, but this change looks ugly to me, in particular for a function that is part of the public API. How about we add an alternative function on the private API that has the ideal format for the current use-case?

Alternatively, we could also consider skipping this function in GridCell and instead copy part of its logic, a bit of code duplication isn't necessarily a bad idea.

[romgrk]

This would be a fairly simple change, and would keep the public API clean:

getCellParams(id, field) {
  return internal_getCellParams(id, field, getRow(id), getRowNode(id))
}
internal_getCellParams(id, field, row, rowNode) {
  /* ... */
}

[lauri865]

Very much agree in general. But since it started from a proof of concept, I didn't want to redesign any apis within its scope to make it a non-breaking change initially, which can surely be improved upon, especially if we're open to breaking changes.

I tried to avoid copying the logic into the component, as then it's way too easy to create a divergence in behavior vs. what we expose in the public API.

The 2nd suggestion makes the most sense, as long as the public API calls the internal one, as you suggested.