Overly aggressive shouldComponentUpdate for the List component

[yihangho]

What you were expecting:
When the body of a ResourceList (e.g., PostList, etc) changes (e.g., due to state change), the changes should be picked up by List and Datagrid, and trigger a UI update.

What happened instead:
The changes in ResourceList are not being picked up by List and Datagrid.

Steps to reproduce:
Referring to the sample code below. Run window.setShowId(false). The ID column should disappear, but it doesn't. Looking at the React Developer Tools, I can confirm that the state is indeed updated.

Related code:

import React, { Component } from 'react';
import { List, Datagrid, TextField } from 'admin-on-rest';

export class PostList extends Component {
  constructor(props) {
    super(props);

    this.state = {
      showId: true
    };
  }

  setShowId = (showId) => {
    this.setState({
      showId
    });
  };

  render() {
    const { props } = this;
    window.setShowId = this.setShowId

    return (
      <List {...props}>
        <Datagrid>
          { this.state.showId && <TextField source="id" /> }
          <TextField source="title" />
        </Datagrid>
      </List>
    );
  }
}

Other information:
This issue is very much inspired by the need to show/hide columns on the resource list page. This has been raised at least twice (#1381, #1386) here and at least once on Stack Overflow (https://stackoverflow.com/q/48211984/211319). @djhi proposed a solution, but it did not work.

I have done some digging and found the bug and a fix. Essentially, List has a pretty aggressive shouldComponentUpdate. Since it does not check if the children have changed, it returns false and no rendering is done. A simple fix here is to always return true in shouldComponentUpdate. (I have tested this and the problem went away.) However, obviously this is not the right way. In fact, I am not sure what is the right way to solve this. I'd be happy to send in a PR to fix this if the core team can give some pointers on this issue.

Notice that this issue can manifest in other forms. For e.g., if one were to customize the properties of the underlying table component using <Datagrid options={ someStateDerivedValue } />, this, too, will fail because of the same issue.

Environment

• Admin-on-rest version: Latest master branch as of time of writing (be1bfa8)
• Last version that did not exhibit the issue (if applicable): It appears that the changed is introduced in 00be45.
• React version: 16.2.0
• Browser: Safari 11 & Chrome 64
• Stack trace (in case of a JS error): N/A

[fzaninotto]

What if you add a key prop in the List component, and change it whenever your side of the state changes (i.e. every time you add or remove a column)?

[yihangho]

@fzaninotto Well that works, but don't think that's a nice way, yes?

[fzaninotto]

The alternative is to check about children, but in your example the change happens in the grandchildren.

[yihangho]

Hmm, in that case probably it makes sense to let the children decide whether to re-render?

[fzaninotto]

no, because in that case a lot of rerenders have already occurred (title, filters, pagination).

[yihangho]

What I mean is that things like title, filters, pagination, and datagrid can implement their own shouldComponentUpdate.

[afilp]

I believe that what you mention is related to my problem too, see below:
#1965

Note: Eventually, adding a key to List (that changes with isShown value) did the refresh. But it seems quite slow because it fetches data again, etc. (while we only need to show/hide columns).

[Kmaschta]

Is this issue still related to the newer versions of react admin?
The #1965 is closed and a workaround has been proposed:

#1965 (comment)

[fzaninotto]

Fixed by #3635