Fix for list not updating when filter prop is changed #2926
Conversation
| prevProps.query.perPage !== query.perPage || | ||
| !isEqual(prevProps.query.filter, query.filter) || | ||
| !isEqual(prevProps.query.filter, query.filter) || | ||
| !isEqual(prevProps.params, params) || |
There was a problem hiding this comment.
you've added the params key here, it has nothing to do with the bug you mentioned. Please remove.
| ? nextProps.query | ||
| : nextProps.params | ||
| ); | ||
| this.updateData(Object.keys(query).length > 0 && query); |
There was a problem hiding this comment.
no, you've changed the logic here. Please revert.
There was a problem hiding this comment.
@fzaninotto this was due to the second bug I mentioned. If you apply the dynamic filter with no query params for example /#/users the params with defaults of null are called on the update data queries. This throws the "Cannot read property 'toLowerCase' of null" error.
Since the params data is merged with the defaults in the this.getQuery() I thought that just not passing the params in this case would still work and properly merge the defaults, but perhaps I misunderstood the logic here?
There was a problem hiding this comment.
with the previous logic, if there is no query string in the URL, we use the params from the Redux store, which are always defined.
I don't know about the Cannot read property 'toLowerCase' of null" error, I suggest we deal with it in another PR, and you provide a failing test case to demonstrate it.
| @@ -222,6 +231,12 @@ export class UnconnectedListController extends Component<Props> { | |||
| nextProps.translate === this.props.translate && | |||
| nextProps.isLoading === this.props.isLoading && | |||
| nextProps.version === this.props.version && | |||
| nextProps.filter === this.props.filter && | |||
| nextProps.params.sort !== this.props.params.sort && | |||
There was a problem hiding this comment.
Same remark as above, don't add the test for params key as it has nothing to do with the bug you're solving.
There was a problem hiding this comment.
@fzaninotto in my testing when I removed the params comparison on shouldComponentUpdate the list failed to immediately update with no query string present. (/#/users)
This caused the sort filter to have to be clicked twice to be applied.
There was a problem hiding this comment.
That means you have broken something in your PR that used to work before. Applications built with the current version have no problem displaying a resource list page without query string in the URL.
|
@djhi @fzaninotto if the logic for the change is accepted I'm happy to add a test for it, but if it needs to be implemented differently I'd appreciate some direction on how you feel I should change it and still resolve the |
| ? nextProps.query | ||
| : nextProps.params | ||
| ); | ||
| this.updateData(Object.keys(query).length > 0 && query); |
There was a problem hiding this comment.
with the previous logic, if there is no query string in the URL, we use the params from the Redux store, which are always defined.
I don't know about the Cannot read property 'toLowerCase' of null" error, I suggest we deal with it in another PR, and you provide a failing test case to demonstrate it.
| @@ -222,6 +231,12 @@ export class UnconnectedListController extends Component<Props> { | |||
| nextProps.translate === this.props.translate && | |||
| nextProps.isLoading === this.props.isLoading && | |||
| nextProps.version === this.props.version && | |||
| nextProps.filter === this.props.filter && | |||
| nextProps.params.sort !== this.props.params.sort && | |||
There was a problem hiding this comment.
That means you have broken something in your PR that used to work before. Applications built with the current version have no problem displaying a resource list page without query string in the URL.
|
@djhi @fzaninotto I've reverted the code as requested. This does expose the |
|
I saw a comment in code that directly points that it will not trigger an update, but this feature is used in almost every page of my current project, so i did the same thing, by ugly ejecting the whole component from ra-core. Please add this to master someday ) |
|
Superseded by #3308 |
fixes #2923
componentDidUpdateinstead ofcomponentWillReceivePropsfilterandparamsto list of props comparison values forshouldComponentUpdate