Mui-datatables: trouble maintaining columns filterList state after re-rendering MuiDatables

[ericpyle]

As far as I can tell, since version 2.1.0, MuiDatatables is having trouble maintaining filterList state during re-rendering when columns[].options maintain their own filterList. Version 2.0.0 seems to work okay.

Expected Behavior

When configuring columns with filterList, re-rendering the MuiDatables component should respect columns' filterList, if columns filterList has changed since table initialization.

Current Behavior

After initializing a column with a filterList, then using the UI to remove the filter (thus emitting filterChanged) results in posting the filterList to higher order component's state, which then propagates back down to rebuild the initial column's filterList. This triggers the MuiDatatable to re-render, but it reverts back to the original filterList rather than 1) the new filterList being passed in through columns or 2) its most recent state as reflected in the filterChanged event. (Effectively undoing the filter change event).

Steps to Reproduce (for bugs)

1. configure columns with non-empty filterList
2. handle onFilterChange to change the state of the current columns filterList

I've simulated this unexpected behavior here (try clicking on the x for Franky Miles to remove that filter. Notice the messages in the console):
https://codesandbox.io/embed/muidatatables-change-columns-onfilterchange-ybhyv

Your Environment

| Tech | Version |
|--------------|---------|
| Material-UI | 3.9.2 |
| MUI-datatables | 2.3.0 |
| React | |
| browser | |
| etc | |

[ericpyle]

By the way, I had thought that one way to avoid this was to not change the initial columns options filterList, and so trust MuiDatatables to manage its own filterList state. However, if I changed a filter from the initial state and then changed the data (e.g. added a new row to the table), the filterList would also revert back to the initial columns options filterList.

You can see an example of how changing the data after changing the filter will just revert back to the earlier filter. Try clicking on the x to delete either of the filters:

https://codesandbox.io/s/muidatatables-change-data-after-onfilterchange-x33ok

[ericpyle]

@gabrielliwerant any idea how hard this is to fix? I'm kind of blocked since this would be a regression from the current behavior of my software. I tried going back to version 2.0.0 which doesn't have this issue, but that version doesn't have searchText which would be another regression if I took it out.

[gabrielliwerant]

I believe this is related to https://github.com/gregnb/mui-datatables/pull/650. You can read my thoughts there to get a sense, but the short answer is that I do believe a permanent fix here is difficult, though there may be a temporary short term fix that is worthwhile. I am working on something more permanent, but if it takes too long, I may end up resorting to temporary hack until I can finish it.

[ericpyle]

@gabrielliwerant thanks for the background. I agree that trying to maintain derived state has led to a lot of bugs and headaches, and I've also begun refactoring my own code to move those to app state (redux), and eventually get into using hooks. I appreciate your movement in that direction. I'm not too afraid of breaking changes as long as the migration steps are clearly outlined :)

[ericpyle]

@gabrielliwerant and temporary hacks are acceptable as well ;-)

[ericpyle]

@gabrielliwerant likely a temporary hack, but this pull request (https://github.com/gregnb/mui-datatables/pull/678) seems to fix this issue

[ericpyle]

@gabrielliwerant nevermind, I'm beginning to understand the code better. so I'll go back to the drawing board. I can see that my fix does not respect the model differences between props.columns and state.columns. It seems the original code is effectively detecting whether the props in setTableData(props) is this.props or something else (e.g. selectRowDelete()). Since rawColumns(this.props.columns) will always equal rawColumns([this.]props.columns), I'm not quite sure if isEqua(rawColumns... is needed. IOW: newColumns === this.props.columns may be sufficient for current code paths. And since even selectRowDelete passes columns: this.props.columns, as far as I can see this long winded condition is only saying "if we have any this.state.columns" then don't rebuild them.

[ericpyle]

@gabrielliwerant fwiw, here are two failing tests that can be used to know if the two issues I've documented in this issue 657 are resolved:

  it('should not update internal column state on data props change after initial columns options filterList', () => {
    const initColumns = cloneDeep(columns);
    initColumns[0].options.filterList = ['Joe James'];
    const mountShallowWrapper = mount(shallow(<MUIDataTable columns={initColumns} data={data} />).get(0));
    const instance = mountShallowWrapper.instance();
    // now disable Joe James filter
    instance.filterUpdate(0, 'Joe James', 'Name', 'dropdown');
    mountShallowWrapper.update();
    mountShallowWrapper.setProps({ data: [] });
    mountShallowWrapper.update();
    // mimic componentDidUpdate(prevProps) {
    // if (this.props.data !== prevProps.data || this.props.columns !== prevProps.columns) {
    instance.setTableData(mountShallowWrapper.props(), 1 /* instance.TABLE_LOAD.INITIAL */);

    const updatedState = mountShallowWrapper.state();
    const { columns: updatedColumns } = updatedState;
    // make sure we still no longer have an active filter
    assert.deepEqual(updatedState.filterList, [[], [], [], [], []]);
  });

  it('should apply columns prop change for filterList', () => {
    const mountShallowWrapper = mount(shallow(<MUIDataTable columns={columns} data={data} />).get(0));
    const instance = mountShallowWrapper.instance();
    instance.initializeTable(mountShallowWrapper.props());
    // now use updated columns props
    const newColumns = cloneDeep(columns);
    newColumns[0].options.filterList = ['Joe James'];
    mountShallowWrapper.setProps({ columns: newColumns });
    mountShallowWrapper.update();
    // mimic componentDidUpdate(prevProps) {
    // if (this.props.data !== prevProps.data || this.props.columns !== prevProps.columns) {
    instance.setTableData(mountShallowWrapper.props(), 1 /* instance.TABLE_LOAD.INITIAL */);

    const updatedState = mountShallowWrapper.state();
    const { columns: updatedColumns } = updatedState;
    assert.deepEqual(updatedState.filterList, [['Joe James'], [], [], [], []]);
  });

[gabrielliwerant]

@ericpyle Thanks for your input. With respect to selectRowDelete, I agree there is some strangeness regarding that part of the code, I noticed it too. But it's difficult to know what to remove at this point, as there is much that still isn't tested, and in particular, anything reliant on the lifecycle hooks is very difficult to verify via testing in my experience.

[ericpyle]

@gabrielliwerant I agree that there's more test coverage needed. It would help those of us who aren't as familiar with the code be able to make pull requests knowing that we aren't creating regressions!

[ericpyle]

@gabrielliwerant I think I'm going to switch my focus away from trying to make [trivial] updates via props, toward making sure I can delay rendering the table until I can initialize it with the needed props, use change function hooks to save changes to disk (which could be used to initialize). Then if/when we migrate to a props oriented approach later, I can revisit. So, I will focus on fixing the above test: "should not update internal column state on data props change after initial columns options filterList"

[gabrielliwerant]

@ericpyle You may have trouble with hooks as the library is not up to the appropriate react version. I am also purposefully delaying the update until the refactor is done, so this may be too much to take at this point. But if you can do something without too many changes, I will be happy to see what you come up with.

[ericpyle]

made new issue #689

[ericpyle]

@gabrielliwerant fixed by https://github.com/gregnb/mui-datatables/pull/711

(The only caveat is that the client must keep track of filterList changes and propagate them back down through column props in order to not lose UI based filter state changes.)