[RFC] Change API state

[oliviertassinari]

Looking at the current shape of the API, it seems that we have an opportunity to explore a more intuitive and flexible API around how the different states can be handled. We have started to uncover this aspect in #133, where we talk about renaming sortModel to sortBy. We have also uncovered some of the problems in #165 (comment) with the update columns method.

Changes I would propose:

1. It doesn't make sense to have a controllable prop and an imperative method that can update a state at the same time. It's confusing, it can lead to nasty bugs when the grid rerenders with the controllable prop and undo the imperative calls. If we take the sortModel state, I think that setSortModel should warn when used without the uncontrollabe API: defaultSortModel.
2. The corollary is that we should introduce a defaultSortModel prop to allow the uncontrollable API to function with the imperative API setSortModel.
3. Update of the defaultSortModel prop could be supported, and equivalent to calling setSortModel.

This gives us:

• sortModel: the controllable prop, will be used no matter what as a source of truth.
• defaultSortModel: the uncontrollable prop, the state is hosted by the component
• setSortModel: imperative API to update the uncontrollable mode, update the state hosted by the component. Throw or warn if the state is already controlled.
• onSortModelChange: callback method used for the controllable mode.
• getSortModel: imperative API to pull the state hosted by the component.

Another corollary is that we should remove duplicated APIs, for instance: https://github.com/mui-org/material-ui-x/blob/91b05ee8b6cc9fb1beab96187fd4308d4b7b294b/packages/grid/x-grid-modules/src/models/colDef/colDef.ts#L56-L63 should be removed for simplicity and clarity.

In the documentation, I think that we should:

• rely on the uncontrollable mode when possible
• have the controllable mode documented before the imperative API

While in this demonstration, I take the "sortModel" state as an example, I think that we can apply the very same model to all the other states. In the core package, there is a useControlled() hook to normalize the behavior. Example of other states we can expose:

• columnsWidth/defaultColumnsWidth/onColumnsWidthChange e.g. https://devexpress.github.io/devextreme-reactive/react/grid/docs/guides/column-resizing/#controlled-mode
• columnsOrder/defaultColumnsOrder/onColumnsOrderChange e.g. https://devexpress.github.io/devextreme-reactive/react/grid/docs/guides/column-reordering/#controlled-mode

This should solve my concerns from #165 (comment). Exposing a resetState publicly doesn't feel right, not at all. From what I understand the boolean influences: caching and internal states. Caching should stay private and internal states should be exposed independently (columnsWidth).

[dtassone]

Beside devexpress, are there many grids and components library using this approach of controlled vs uncontrolled?
This seems to be a concern of forms mainly.
In terms of refactoring the state, I'm thinking of doing a sort of redux approach with a central state like a store and and every hook would register an entry on it. So each hook have an isolated state in the store.
I think it would be great for debugging and the idea is to hook it to react dev tools and maybe do a redux adapter.
Lastly, it is to make it easier to persist the grid state at any given time.

[oliviertassinari]

The initial proposal on the issue definitely work great when there are a limited number of states, like less than 5. For instance, TreeView has 2 useControlled(), Autocomplete has 3 useControlled(). However, the data grid will have a lot more. Should we consider a different solution?

Beside devexpress

I don't think that we should consider the problem with a lens on a data grid, but on how to handle the internal states of React components. (so be React focused, not grid focused). In any case, I think that we should lever break the uncontrolled/controlled pattern.

In terms of refactoring the state

The approach you are suggesting sounds close to the "state reducer": e.g. https://github.com/downshift-js/downshift#statereducer or https://react-table.tanstack.com/docs/api/useTable#table-options. In this mode, the component seems to have:

• For the uncontrollable mode

  • stateReducer. This allows customizing the internal logic.
  • defaultState to set the initial state.

• For the controllable mode, there is a single useControlled for all the states, not for each of them individually.

  • state.
  • onStateChange.

  Optionally a useControllableState, but I don't see what problem is solves that state/onStateChange can't.

This approach sounds promising to scale internal states :)

[m4theushw]

setSortModel: imperative API to update the uncontrollable mode, update the state hosted by the component. Throw or warn if the state is already controlled.

@oliviertassinari Maybe we need to do some check at the setGridState level because this API is also used internally. As an example, in #1719 (comment) the state updates even when on controlled mode.

[oliviertassinari]

@m4theushw Don't take my previous comments here too seriously. They were more ideas than actual recommendations that could work.

Yeah, in the pagination, the imperative API is used to trigger the "on prop change" event. We could keep this, it's interesting if you want to use the apiRef to bind your own pagination component while controlling the prop in a parent of the data grid.

So it makes me think that there are actually two completely different use cases for the apiRef, one to bind custom child components, and one to perform actions on the grid from the parent.

[oliviertassinari]

An update:

• We have normalized the controllable API to be idiomatic in [DataGrid] Implement control state/prop for all props using useGridControlState hook #2044.
• We have decided to wait for the defaultX prop, to see if we really need it. We can always add it later if the pain is raised.
• For the naming of the props, we often (66%) use "Model" as a suffix. It's unclear why. It doesn't have any terminology anchoring in React. We could/should remove this suffix. But in any case, it could be done later on and is not really important.
• The imperative APIs are treated as sources of event. Meaning they are designed to handle user events, like clicks.
• The state/onStateChange props are considered private because many parts of the state is private. We can't expose it all as is. Not yet.
• We didn't explore the stateReducer/useControllableState pattern, we haven't yet seen people raise problems that ask for it. It might be overkill, controlled props might be all developers need. The closest we did is to allow disabling the native handler of the data grid for a specific event.