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

Rows are modified with a new property tableData #666

Closed
paillave opened this issue Jun 3, 2019 · 20 comments
Closed

Rows are modified with a new property tableData #666

paillave opened this issue Jun 3, 2019 · 20 comments
Assignees
Labels
question Further information is requested wontfix This will not be worked on

Comments

@paillave
Copy link

paillave commented Jun 3, 2019

Describe the bug
Might be related to #269 as I have exactly the same pb.
When rendering rows in the grid, the property tableData is added to every row. This is a big problem with redux as its state MUST never be changed. Therefore, issue mentioning a readonly property may come from the fact that immutability frameworks like immer lock objects from the state for them not to be amended afterwards (as redux demands reducers and components to be pure).

To Reproduce
render a list of value in the grid

Expected behavior
at the moment, before the rendering, business objects are directly manipulated by adding a tableData metadata on them:

{
  ...businessRow,
  tableData:{
    id:0
  }
}

what may be better instead would be to keep on behaving this way by default, but to permit to get the identifier from the row. If such an option is given, then, instead of getting/setting this "tableData" on the row itself, it can be recovered from a dictionary that is stored in the state.

This way, the input business row is never touched and stored as is in the object that is used by the rendered, and as a sibling property you have all the useful metadata for the rendering purpose.

As ideally, react component should be pure (not modifying its properties), this amendment would help a lot, at moreover it would permit your grid to work with react in any situation.

@paillave
Copy link
Author

paillave commented Jun 4, 2019

As it is a big pb at least for me, I am working on it on my branch. If you want, I can make a pull request once it's done.

@mbrn
Copy link
Owner

mbrn commented Jun 5, 2019

Hi @paillave ,

MT tracks many changes on rows and columns. So i had to change rows and columns. But we may try to add a flag to options make rows and columns immutable. But many features will not work. I am not sure to do this.

@mbrn mbrn self-assigned this Jun 5, 2019
@mbrn mbrn added the question Further information is requested label Jun 5, 2019
@paillave
Copy link
Author

paillave commented Jun 5, 2019

I am working on a solution, I'll make a pull request once it is done. It is an amendment this will require a lot of tests as this property is really used a lot.

@mbrn
Copy link
Owner

mbrn commented Jun 5, 2019

Of course. I can check it after completed.

@masbaehr
Copy link

I also encountered this issue. Since it seems to take quite a while - i propose a workaround:
At the point where you need to return the original data set do this:

 function getList(){
      var returnData = [];
      state.data.forEach(element => {
          var elCopy = Object.assign({}, element);
          delete elCopy["tableData"];
          returnData.push(elCopy);
      });
      return returnData;
  }

If there is a nicer way feel free to improve it ;-)

@paillave
Copy link
Author

paillave commented Jun 18, 2019

That's what I did, but the grid actually needs this data. Otherwise, the whole context of what the user made against it is forgotten when the reducer (for example) amends a row in the input data set.

@fargraph
Copy link

Would it be possible to do a cloneDeep() of the passed in data so that you're not modifying the original data?

@jdmairs
Copy link

jdmairs commented Oct 22, 2019

I just ran into this too...perhaps the docs should post a warning if you are using Redux.

@techyrajeev
Copy link

It is adding extra property with redux state. How can it be avoided?

@cyberprodigy
Copy link

Same here, material table seems to be incompatible with immer. I guess material table should not mutate original state anyways

@braco
Copy link

braco commented Mar 12, 2020

This bug is still going on. I have different data views, when switching from table to anything else it has a new unexpected property. Should not be modifying the original data!

I am thinking about switching to this (see Material example toward end):
https://github.com/tannerlinsley/react-table/blob/master/docs/examples.md

@masbaehr
Copy link

Another solution you might try. I'm using it without too much issues. Just be careful if you have circular dependencies or some non-serializable stuff

data={JSON.parse(JSON.stringify(tabledata))}

@HonzaTuron
Copy link

HonzaTuron commented Mar 13, 2020

Or even simpler and faster than JSON stringify and parse:

data={data.map(o => ({ ...o }))}

Works like a charm for me.

@cutamar
Copy link

cutamar commented Apr 17, 2020

Those are no solutions, but workarounds. Using the new hook api is impossible when the data gets mutated.

@superjose
Copy link

@HonzaTuron Nice. It may be slightly hacky, but I can confirm it's working with a reducer.

@ReshaSD
Copy link

ReshaSD commented Jul 18, 2020

Those are no solutions, but workarounds. Using the new hook api is impossible when the data gets mutated.

@cutamar , you can try something like this to wrap your data:

  const workaroundData = useCallback(async query => ({
    data: fetchedData.map(r => ({ ...r })),
    page: query?.page,
    totalCount: fetchedData.length
  }), [fetchedData]);

Edit material-table-immer

@ifndefdeadmau5
Copy link
Contributor

ifndefdeadmau5 commented Sep 7, 2020

Any progress on this? as much as redux, with react-apollo this behavior also causes similar problem
e.g) checked rows are still remain checked after component gets unmount, as it directly affects to apollo cache - so 'no-cache' fetch policy helps in this case :)

@GalumphingCallooh
Copy link

Ha! This issue might have cost me a job interview in a demo project, but the good news is that I have a fix for this in my local branch. I've used ES6 Symbols to decorate the data object with a [tableData] symbol that's only used internally in material-table. To support older browsers I've also included a polyfill. Let me submit a pull, give me a little time to clean up & check it as I was fixing it haphazardly.

@Domino987
Copy link
Collaborator

Hi this problem is long know and these fixes where already thought and tested, but the main probkem is, that some users rely on object reference to find matching objects in their data during callbacks. That's why @mbrn rolled back a merged PR to exactly fix this #1174. I think we should either find a way to keep both the old reference data and not mutate or create a breaking change that obj reference is not guaranty anymore.

@stale
Copy link

stale bot commented Jan 4, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. You can reopen it if it required.

@stale stale bot added the wontfix This will not be worked on label Jan 4, 2021
@stale stale bot closed this as completed Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests