Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove data mutation #1174

Merged
merged 2 commits into from
Oct 15, 2019
Merged

Remove data mutation #1174

merged 2 commits into from
Oct 15, 2019

Conversation

Domino987
Copy link
Collaborator

@Domino987 Domino987 commented Oct 10, 2019

Related Issue

#1014

Description

This PR removes the mutation of the provided data by copying the provided data and not writing to the provided props directly.

This is in line with the best practices since props should never be mutated and this causes unforeseen side effect. If the data comes from redux or another state management library, these changes persist over the lifetime of the session and might even be played back into the backend if the data is updated and the tabledata is passed as well, since the global object is changed.

This also makes it difficult to reset the selection if the same data is used for multiple tables in which case selection in one table effects the selection of the other, even so they should be separated.

Additionally, the mutation is not mentioned in the documentation which makes these changes also unpredictable.

I am aware that this change will reset the selection, the extended panels, edits etc. on data change, but this is in my opinion not a problem since the table should reset itself, if new data is received. And if it is needed to persist the selection, one could use the tableRef to access the data and use that to keep the selection.

Let me know what you think and if you agree with me so we can adress this issue.

Impacted Areas in Application

List general components of the application that this PR will affect:

  • Data-Manager

Additional Notes

I would really like this change to happen, since it makes the api more predictable and does not mutate my data ;)

This PR removes the mutation of the provided data by copying the provided data and not writing to the provided props directly.
@jayarjo jayarjo added this to the v1.53.0 milestone Oct 12, 2019
@mbrn mbrn merged commit 321fa8c into mbrn:master Oct 15, 2019
oOo-mika-oOo pushed a commit to oOo-mika-oOo/material-table that referenced this pull request Oct 19, 2019
@see Remove data mutation mbrn#1174
The tableData was not persisted .
On each redraw, the table was restored in his initial state.

2 solutions implemented :
- legacy, keeping datamutation,
- new version, needs dataFieldId props attribute. a Map in data-manager bind the id to corresponding tableData
@Domino987
Copy link
Collaborator Author

Hi @jpmtrabbold , Yes it may be a breaking change for some as I described in the commit description. If you rely on the tableData to persist or something similar it may change. The users should not rely that onRowClick returns the same object. What is your use case, that makes it necessary for it to be that same object? Can you describe it, so that we can work on a solution?

mbrn added a commit that referenced this pull request Nov 8, 2019
@Domino987
Copy link
Collaborator Author

hi @jpmtrabbold , can you tell me about your use case that you need the exact reference to the original data object?

@jpmtrabbold
Copy link

Let's say I have:
source of truth: 10 rows
filtered data used with material-table: 4 rows (out of the 10 original)

User clicks on that row, and I receive the onRowClick callback. I need to know what's the original item in the source of truth, so I use sourceOfTruthArray.indexOf(onRowClickItem).

I know that I could solve that just by using ids, but we have too many places using that.

Also, it's expected that the callback should hold the same reference. This week I wrote a component to replace material-table in our use cases, and I found a great solution for both not mutating the props data and also not having separate references for the component (waste of memory), it's working pretty well.

All in all, don't worry about me - if you are going to change it, do it for the sake of having a more predictable component because I'm not depending on it anymore. Keep up the good work!! Cheers bro

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants