Updated table to make state persistent between renders

[patorjk]

This PR updates the table so that it maintains it's state between re-renders for properties that are not overwritten by the user (#807, #947, #1057). I've updated the "Simple" and "Data as Objects" examples with a "re-render" button that shows that search text, filters, selections, sorting, etc etc, are not lost when the table re-renders.

Some notes:

• Selected Rows and Expanded Rows reset if the data changes. This is why I've put the data on the component's state for the 2 examples I updated. In my own use, I found that if the table's data changed, it made no sense for the selections and expansions to stay in place.
• The Trash Can in the toolbar doesn't effect the external state data in those two examples, so any deleted data is returned to the table on re-render. This makes sense because the user is inputing non-modified data to the component, so the table should use the non-modified data rather than internal data that is modified.
• I'm not sure of a good way to test this. I've copied over my edits from the mui-dt repo I mentioned in the other thread, and I'd been using the changes over there in my app without issue.

[coveralls]

Coverage increased (+0.6%) to 77.044% when pulling 912bf2e on patorjk:persistent_state into 4f83609 on gregnb:master.

[plourenco]

Hey @patorjk ,
Correct me if mistaken, but isn't this more like a quick-fix to the table state lost across re-renderings? I would assume the best solution would be the one you suggested in #679 (#679 (comment)).
In other words, breaking the table state into smaller components, where we would pass both the state and a method to update it.

I see this working just fine, except if you need to control one of the tableOptions and have no way of notifying the parent about changes, rather than the burden of implementing the onTableChange.

[patorjk]

Correct me if mistaken, but isn't this more like a quick-fix to the table state lost across re-renderings?

No, this would be a permanent fix. Basically it makes it so state the table is managing isn't erased.

I would assume the best solution would be the one you suggested in #679 (#679 (comment)).

That was my opinion a few months ago, but the more I've used the table, the more I'm not so sure. That would be a rather big API change and there are no clear benefits as a user - in fact, the API would become more complex and possibly more confusing. And I've never seen an API that looks like what I suggested. Re-reading [1], it doesn't say derived state shouldn't be used, it just says it's an advanced feature and that in many cases you don't need it.

I noticed at the bottom of the article it invites questions, so I've gone ahead and put in a question regarding this particular situation. Maybe someone there will have some insight on the best way to approach this type of problem.

However, from own experience, using derived state this way - as long as you know what you're doing - seems to be fine. It also keeps the complexity under the hood so that users don't have to deal with it. That's partly why people are having so many problems, when they have to manage the state themselves, it becomes a headache.

have no way of notifying the parent about changes, rather than the burden of implementing the onTableChange.

I don't see this as an issue. Event handlers are traditionally used for this type of thing.

[1] https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html

[plourenco]

I think I was deviating a little bit from the main subject of this PR. I agree with the PR fix to have the table manage its own internal state when the props are not set/undefined. I'm also doing it myself. However, my current concern is regarding the table state that is set and controlled.

I don't see this as an issue. Event handlers are traditionally used for this type of thing.

If I wanted to control the sortDirection for a certain column, I would have to go into doing something like this:

const columns = {
    name: 'foo',
    label: 'bar',
    options: {
        sortDirection: this.state.firstColumnSortDirection
    }
};

const options = {
    onTableChange: (action, tableState) => {
        if (action === 'sort') {
            if (tableState.columns[0].sortDirection !== this.state.firstColumnSortDirection) {
                this.setState({ firstColumnSortDirection: tableState.columns[0].sortDirection });
               // Note: Also, when the sortDirection is none, we're suppose to delete it, not even send it as props.
            }
         }
     }
};

While a simpler approach could be:

const columns = {
    name: 'foo',
    label: 'bar',
    options: {
        sortDirection: this.state.firstColumnSortDirection,
        onChangeSortDirection: (dir) => this.setState({ firstSortDirection: dir })
       // Or something else that includes this callback as props
    }
};

Plus, using the onTableChange is causing an extra re-render on the parent, considering the child has already updated its own internal state.
However, I believe this problem is already sufficiently exposed in #679 and should be a separate PR.

[patorjk]

That's a good point and I forgot about that - yes, this fix solves the state loss problem for uncontrolled props but doesn't address that controlled state still has the "Double Render" problem (I'll call it that to make it easy to refer to). The Double Render problem would only be solved by an internal rewrite and API change.

In that sense, this PR would be a stop gap until #679 is completed.

[Legogris]

Can confirm that this fixes our issues with filters dropping between mutations of table data (for context, using a non-rendered meta column to keep track of client-side editing state per row).

[flvyu]

Can anyone review and approve this, or do we need someone specific to get this merged?

[aamin21]

@patorjk You probably have seen this, but there is a conflict with the PR.
@gabrielliwerant Can anyone do code review and approve or are there particular maintainers that need to approve for this to be merged?

Thanks for you work guys!

[patorjk]

@aamin21 I'm using a fork of this repo that's on my client's servers, it's in current development but it's unavailable to the public. I'd prefer to use this repo, but none of the PRs I submitted months ago were reviewed. I'm not going to submit any more changes if they're not going to get looked at (though I've set this PR up so that maintainers can do any changes they want to it). This update is really important though, even if @gabrielliwerant has other plans for the future, in the current state, the loss of state on re-render is unexpected and leads to bugs.

Also, PR #1088 is really important too, as this table is broken in Safari (see #900 for details) and on Chrome/Firefox on iPhone.

I talked to @gregnb on twitter a couple months ago and he expressed interest in coming back, but I understand why he stepped back (he had personal stuff to deal with). I think maybe if he came back, he could help excellerate the development that @gabrielliwerant is working on, but other than that, fixing this issue is in the hands of @gabrielliwerant.

[jaekunchoi]

Can we merge this one in? This is causing problems for me also with filters getting reset

[jaekunchoi]

LGTM

[occult]

Looks good! Can we approve this? Is there anything left to approve this? I can help :)

[alycoy]

LGTM

[hacknaked]

I still see this issue in v3.5.0