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.04k stars 1.24k forks source link

[data grid] Document `onStateChange` prop #8582

Open AndreSilva1993 opened 1 year ago

AndreSilva1993 commented 1 year ago

Order ID or Support key 💳 (optional)

41693

Duplicates

Latest version

The problem in depth 🔍

Hi all! I've searched everywhere for my use case but didn't seem to find it nowhere and just want to be sure I'm not shooting myself in the foot.

We have a lot of datagrids in our project and want to persist its state in local storage so that the user can resume its workflow when navigating back to the same page(s). However I feel that tapping into each individual method / event listener that might change relevant information that should be stored and calling the exportState there would become way too repetitive.

Would something like this be viable? I tested it and it works but I just want to be sure that memoizing / holding onto the apiRef.current to use it in the cleanup function does not bring any issues. Without doing this, in the effect's cleanup function the apiRef.current is already null.

export function usePersistedGridState(apiRef) {
  useEffect(() => {
    const gridRef = apiRef.current;

    return () => {
      console.log(gridRef.getAllColumns()); // save the columns visibility
      console.log(gridRef.exportState()); // save the grid's state
    };
  }, []);
}

Your environment 🌎

`npx @mui/envinfo` ``` `npx @mui/envinfo` hangs indefinitely for me but these are our versions: "@mui/lab": "^5.0.0-alpha.104", "@mui/material": "^5.10.10", "@mui/styles": "^5.10.10", "@mui/system": "^5.10.10", "@mui/x-data-grid-pro": "^5.17.18", ```
yaredtsy commented 1 year ago

hey @AndreSilva1993

cleanup functions are not guaranteed to be called in all scenarios. there is some condition it might not be called. so I do not think it's a good idea.

I feel that tapping into each individual method / event listener that might change relevant information that should be stored and calling the exportState there would become way too repetitive.

exportState export the state, so you only need to subscribe to stateChange event. and you do not have to use getAllColumns to save the column visibility. getAllColumns is just a selector function. it's selecting the columns from state. exportState is enough.

AndreSilva1993 commented 1 year ago

Hi @yaredtsy, first of all thanks for the swift response. We are using the onStateChange event but the fact that it is invoked way too often (we're using a debouncing mechanism) and the fact that it is an undocumented event, it led me to believe that we didn't have the best approach to this.

Also the getColumns there was to circumvent this issue - https://github.com/mui/mui-x/issues/4620 - since we can't migrate to v6 yet and I was getting inconsistent exports from the exportState which does not always include the columnsVisibilityModel.

So the advised approach in this scenario is to use the onStateChange then?

ETA: The fact that our onStateChange is being invoked way too often falls on our backs though.

yaredtsy commented 1 year ago

We are using the onStateChange event but the fact that it is invoked way too often (we're using a debouncing mechanism

yes sometimes it gets triggered unnecessarily, and also the state that is changed might not be exportable state so use deep equal, to check if the exportable state is changed. only then save the state.

const prev = React.useRef();
 <DataGrid
     ....
      onStateChange={(e) => {
          const current = apiRef.current.exportState();
          if (!_.isEqual(prev.current, current)) {
            prev.current = current;
            //save the state here
          }
        }}
 />
yaredtsy commented 1 year ago

Also the getColumns there was to circumvent this issue - https://github.com/mui/mui-x/issues/4620 - since we can't migrate to v6 yet and I was getting inconsistent exports from the exportState which does not always include the columnsVisibilityModel.

have you tested it with the latest version it seems to be working fine here: https://v5.mui.com/x/react-data-grid/state/#restore-the-state-with-initialstate. and I have also checked the code and the fix has been applied to V5, it should work.

AndreSilva1993 commented 1 year ago

@yaredtsy It does work indeed, we were on v5 a few PATCH versions behind. Ok will use the onStateChange with the deep equal comparison and since exportState returns everything I need, there's no need for other selectors. Feel free to mark this issue as solved and thank you so much for the help, it's really appreciated.

m4theushw commented 1 year ago

onStateChange is not documented because until now we didn't have a use for it. I think we can make it stable.

AndreSilva1993 commented 1 year ago

@m4theushw that'd be awesome! I've seen some issues / comments mentioning this event so it would give developers more confidence in using it if it's documented. Thanks!

jonathandewitt-dev commented 1 year ago

While you're at it, why not add a feature that automatically persists the grid state for you? For example, I'm using the id prop as a key to save the grid state in local storage, but you could use IndexedDB or something. Would reduce quite a lot of boilerplate...

clarson0 commented 2 months ago

built in state persistence would be a great feature, we've been having quite a bit of trouble with this, especially when columns change (which the documentation says needs to be stable), but there are cases where the columns have value getters that have dependencies. If there's an "easy" way to deal with that, i'd love to hear it