gregnb / mui-datatables

Datatables for React using Material-UI
MIT License
2.7k stars 931 forks source link

Changing state with setState in onRowExpand collapses the expanded row #989

Open keybraker opened 4 years ago

keybraker commented 4 years ago

All three versions try to showcase a general problem in the functionality of expandable rows. I was trying to make the program call for a get request from server and update the state of the data in the current expandable row. But although fetching the data correctly and storing it, and being displayed on screen for a second the setState itself was the cause that collapsed the expanded row.

As shown bellow three different ways that a setState is being called cause the same problem to occur.

[1] onTableChange: (action, tableState) => { switch (action) { case 'expandRow': this.setState({age: 1}) break; default: break; } }

[2] onRowsExpand: (curExpanded, allExpanded) => { this.setState({age: 1}) }

[3] onRowClick: (rowData, rowState) => { this.setState({age: 1}) }

gabrielliwerant commented 4 years ago

Hey @keybraker, thanks for reporting. It's a bit of a deep issue, as it's related to the table being a bit confused about when an update has occurred vs initializing state from props. I think I have a workable solution, but the tricky bit is making sure it doesn't break anything else, as it requires changing some internals. I'll link here when I have a PR and I'd be happy to have you take a look through it if you want.

gabrielliwerant commented 4 years ago

Also, note that although there is a bug here regarding generic component updates resetting internal state for expanded rows (I also think filters and selected rows might be affected), row selecting and row expanding is not async behavior currently, so you can't cause them to wait for your actions before they will trigger internal state changes (though you can use promises to change props once it resolves).

keybraker commented 4 years ago

I am using promises, from the example on how to make GET requests, but the problem still remains, as it updates the data of the expandable row but does so by refreshing the whole data table.

gabrielliwerant commented 4 years ago

Yes, the problem still exists as no fix has been pushed, my note about promises was just to explain that you have two issues:

  1. Changing state is refreshing the table
  2. Async does not work with expandable rows

I'm working on 1. but not 2. so I'm advising you on what will happen if you try to do anything about 2.

Also, as I've been diving into this issue, I'm actually finding it's another manifestation of the issue outlined here: https://github.com/gregnb/mui-datatables/issues/679, which means there can't be any true fix for the time being. If you update state, the data will be refreshed, and you'll have to keep it up to date yourself, until there's a new set of APIs that will allow you to control what happens. I'm considering adding an experimental API in the mean time, but thinking about the best way to go about this.

keybraker commented 4 years ago

The functionality I need can be understood by the following example:

A list of cars, is rendered by the table, and for every car's details you have to drop the expandable row. But as a more performance oriented approach, you get all the cars and when a certain car is selected/expanded, you make the call to the database, to get the details.

1) An approach would be to load everything from the beginning, but throwing performance out the window.

2) I could create a button you have to press to fetch the data, but it would be counter intuitive.

So I have kept it this way, meaning it closes the first time you press it and then you press it once more and it expands, until a proper fix id in place. Do you have a better solution ?

gabrielliwerant commented 4 years ago

If you don't mind expanding the row and having a loading indicator in there until your request is successful, then that will fix problem 2 for you, but it still won't address problem 1, which just can't be done at the moment.

For what it's worth, it's not clear to me on hearing your use case that performance will be automatically improved by sending new requests every time a row is expanded. What I might do in your shoes is pull all the data for two pages of the table up front. You have the data for the first page, and you have the second queued up in anticipation of the user hitting the next page. Then you load the 3rd and 4th in the background, as the user pages forward and it continues. If you're retrieving basic json responses from your server then the limiting factor is most likely the number of requests you make, not the size of data. You need really large data sets before you save on performance by making small requests. In most cases, the penalty of the request itself, going through all the http layers back and forth and hitting the database, would be much worse (you can also benefit from caching strategies more easily). Just food for thought. This approach might also make it easier for you to manage state, as you'd only be making these changes when the user pages forward (though it might reset the expanded rows on previous pages).

abdu355 commented 4 years ago

Came up with a workaround that keeps track of the rows open, such that when the data is loaded asynchronously, the expanded rows array ( rowsExpanded ) is used instead, any re-renders will not close the expanded rows, unless you tell it to.

Data table options

` const options = { rowsExpanded: this.state.rowsExpanded, expandableRows: true, onRowsExpand: (currentRowsExpanded, allRowsExpanded) => { //console.log(currentRowsExpanded, allRowsExpanded); this.updateRowsExpanded(allRowsExpanded); }, renderExpandableRow: (rowData, rowMeta) => { const colSpan = rowData.length + 1; return (

    );
  }
};`

UpdateRowsExpanded method:

updateRowsExpanded(allRowsExpanded) { var rowsExpanded = _.map(allRowsExpanded, item => { return item.index; }); this.setState({ rowsExpanded }); //simple value }

Monitor row updates:

componentDidUpdate(prevProps, prevState) { if (prevState.rowsExpanded !== this.state.rowsExpanded) { console.log(this.state.rowsExpanded); } }

rowsExpanded

MikeShobe commented 4 years ago

Any update on this one? I'm having the same issue with setState in onRowsSelect, I'm trying to modify my multiple selection options based on which items are being selected.

patorjk commented 4 years ago

This PR would fix the issue: https://github.com/gregnb/mui-datatables/pull/1086

A current workaround for this would be to save all the table-state information you need in an external object and then plug it into the options object for the table. In this case the rowsExpanded array would need to be saved off and re-placed into the table to avoid the re-render problem.

More discussion on this issue can be found here: https://github.com/gregnb/mui-datatables/issues/807

gowthamrajmpuc commented 4 years ago

Hi how to setState onRowClick ?