Bug Report
- Package version(s): 5.34.0 (current github code has this issue)
- Browser and OS versions: Any browser, any OS
Priorities and help requested (not applicable if asking question):
Are you willing to submit a PR to fix? Yes, but I don't what the fix should be
Requested priority: Normal
Products/sites affected: (if applicable)
Describe the issue:
There is this check in List.tsx's shouldComponentUpdate
if (newProps.items === this.props.items && ....)
The problem with this is this that this just compares the memory location that is pointed to by newProps.items, and this.props.items. I was facing an issue where I have an array, and I went ahead and updated an entry within the same array, and passed it down to the list. Now since I didn't recreate the array, the memory location it points to is the same as before. They are pointing to the same array.
Expected behavior:
I am not sure what is the correct solution. As in my case, since the props objects are the same, there can never possibly be any check to distinguish between the two. When one newProps.items changes in any way, this.props.items changes along with it. The only thing that I was wondering is whether the props object should be deep extended and stored (not sure which component lifecycle could help here, or whether any would help at all), so that the issue of holding the same memory reference is avoided. This would enable correct comparisons between the old props and the new props. Another option is to remove the check on this array altogether.
Actionable items
(Copied from Cliff Koh's comment below)
For performance reason, the most sensible thing for the list to do here, by default, here is to behave as if the list item arrays are pure/immutable. The action item here is to document this expectation.
If there is a deep need to have a deep compare, it should at most be provided on a opt-in basis (like a isPure type of flag), or better still, allow users to provide the comparison function.
Bug Report
Priorities and help requested (not applicable if asking question):
Are you willing to submit a PR to fix? Yes, but I don't what the fix should be
Requested priority: Normal
Products/sites affected: (if applicable)
Describe the issue:
There is this check in List.tsx's shouldComponentUpdate
The problem with this is this that this just compares the memory location that is pointed to by newProps.items, and this.props.items. I was facing an issue where I have an array, and I went ahead and updated an entry within the same array, and passed it down to the list. Now since I didn't recreate the array, the memory location it points to is the same as before. They are pointing to the same array.
Expected behavior:
I am not sure what is the correct solution. As in my case, since the props objects are the same, there can never possibly be any check to distinguish between the two. When one newProps.items changes in any way, this.props.items changes along with it. The only thing that I was wondering is whether the props object should be deep extended and stored (not sure which component lifecycle could help here, or whether any would help at all), so that the issue of holding the same memory reference is avoided. This would enable correct comparisons between the old props and the new props. Another option is to remove the check on this array altogether.
Actionable items
(Copied from Cliff Koh's comment below)
For performance reason, the most sensible thing for the list to do here, by default, here is to behave as if the list item arrays are pure/immutable. The action item here is to document this expectation.
If there is a deep need to have a deep compare, it should at most be provided on a opt-in basis (like a isPure type of flag), or better still, allow users to provide the comparison function.