Skip to content

Commit

Permalink
Remove data mutation
Browse files Browse the repository at this point in the history 
This PR removes the mutation of the provided data by copying the provided data and not writing to the provided props directly.
  • Loading branch information
@Domino987
Domino987 committed Oct 10, 2019
1 parent 2dd0ca7 commit ae62214
Showing 1 changed file with 7 additions and 4 deletions.
11 changes: 7 additions & 4 deletions src/utils/data-manager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,14 @@ export default class DataManager {
this.selectedCount = 0;

this.data = data.map((row, index) => {
row.tableData = { ...row.tableData, id: index };
if (row.tableData.checked) {
const localRow = {
...row,
tableData: { ...row.tableData, id: index }
}
if (localRow.tableData.checked) {
this.selectedCount++;
}
return row;
return localRow;
});

this.filtered = false;
Expand Down Expand Up @@ -818,4 +821,4 @@ export default class DataManager {

this.paged = true;
}
}
}

4 comments on commit ae62214

@jpmtrabbold
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Domino987. With this change, this.data is not holding the original references to the original objects inside the array. In line 50, you are deconstructing row (...row), which also means that if it was a named object, it becomes just a plain js object. The problem with that is:

  • this is a breaking change;
  • whenever you pass back the row (for example, to onRowClick), any developer will expect that to be the exact object that he passed before (as it was before this commit), but now this is a completely different reference.

Was that supposed to be a breaking change, by design?

@mbrn
Copy link
Owner

@mbrn mbrn commented on ae62214 Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Domino987

Why did you make this change? What happened if we revert this?

@Domino987
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said in the commit message, it may be a breaking change and nothing happens if it gets reverted. The problem is, that material table mutates the data and the goal was to prevent that. Check out the commit message. Maybe we can find a better or other way.

@mbrn
Copy link
Owner

@mbrn mbrn commented on ae62214 Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now i will revert the commit. We should consider a better solution for both data and columns props.

Please sign in to comment.