columns prop is mutated

[anthonyblond]

Describe the bug
The columns prop is modified.
The reactjs documentation states:

All React components must act like pure functions with respect to their props.

Apologies if I've misunderstood this, I'm relatively new to React. I'm not 100% that this just means the the object a prop references shouldn't itself be modified (shallow), or whether none of the objects it references to should be modified either (deep). I've assumed the latter.

To Reproduce
Steps to reproduce the behavior (sorry, this might not be clear, its just how I noticed it in the first place)

1. Have a MaterialTable component such that its columns definition is declared outside the scope of the its parent's scope.
2. If that table is unmounted and mounted, the text entered into the filters is retained.

Expected behavior
The text should be cleared, as its should be stored in the MaterialTable component's own state.

Alternatively
Use chrome debugger on the columns prop passed in from the parent. Each element in the array has tableData property added to it containing something like:

tableData:
additionalWidth: 0
columnOrder: 1
filterValue: undefined
groupOrder: undefined
groupSort: "asc"
id: 1
initialWidth: "calc((100% - (0px)) / 4)"
width: "calc((100% - (0px)) / 4)"

Additional context
@material-table/core 2.3.37

[oze4]

Hi would you mind creating a Sandbox of this issue?

[oze4]

Also, the tableData prop is used internally and is there by design. That is working as expected.

[peterkmurphy]

Good day, @oze4 . I am a colleague of @unfinishedteleporter . I was looking into the issue myself. So I have created a Sandbox on the issue at:

https://codesandbox.io/s/happy-sara-82gf6

You have App.js

import "./styles.css";
import MaterialTable from "@material-table/core";
import {RECIPIENTS_TABLE_COLUMNS} from './import'

export default function App() {


  const data = [
    { Name: "A", Email: "a@something.com", Status: "Bad" },
    { Name: "B", Email: "b@something.com", Status: "Mediocre" },
    { Name: "C", Email: "C@something.com", Status: "Good" }
  ];
  console.log(RECIPIENTS_TABLE_COLUMNS)
  return (
    <div className="App">
      <h1>Hello Material Table</h1>
      <MaterialTable columns={RECIPIENTS_TABLE_COLUMNS} data={data}
            options={{
              filtering: true,
              toolbar: false,
            }} />
    </div>
  );
}

Importing RECIPIENTS_TABLE_COLUMNS from import.js:

export const RECIPIENTS_TABLE_COLUMNS = [
  {
    title: "Name",
    field: "Name",
    align: "left",
    filterPlaceholder: 'Filter by name'
  },
  {
    title: "Email",
    field: "Email",
    align: "left",
    filterPlaceholder: 'Filter by email'
  },
  {
    title: "Status",
    field: "Status",
    align: "left",
    filterPlaceholder: 'Filter by status'
  }
];

When I look at the console.log, where I see the result of RECIPIENTS_TABLE_COLUMNS, I see:

(3) [Object, Object, Object]
0: Object
title: "Name"
field: "Name"
align: "left"
filterPlaceholder: "Filter by name"
tableData: {
columnOrder: 0
filterValue: undefined
groupOrder: undefined
groupSort: "asc"
width: "calc((100% - (0px)) / 3)"
initialWidth: "calc((100% - (0px)) / 3)"
additionalWidth: 0
id: 0}

And so on.

Yes, it appears that RECIPIENTS_TABLE_COLUMNS is being mutated, despite being declared a const. And this behavior only occurs when RECIPIENTS_TABLE_COLUMNS is imported from another file.

Isn't mutating props a big, big, big problem in the React world? Props should be immutable, period. You have an internal state that you can do what you like, but anything passed into MaterialTable as a prop should be read-only, I thought.

[oze4]

Hey @peterkmurphy completely agree with you! @Domino987 and I were just discussing this. We are not the original authors but we agree. Modifying these props is a no-no and we should only be passing "modified props" (aka copies) back to the caller via, well, callback args.

We shouldn't actually be modifying the props, like it appears we are doing.

The first "phase" of a resolution is going to be introducing redux-toolkit to better handle internal state and fix issues just like this. Right now the code-base is a little messy and hard to follow.

Thanks for this!

[oze4]

Also, I get the same behavior whether imported or in the same file..

[Domino987]

Hi people 🙂
This problem is actually even more severe, since it applies to the data prop as well, so sharing state between tables is also error prone.

Thus is actually a problem since a few years and my fix was reverted, since a few people used object refence to find objects and this broke their code. That using object refence for finding the object is a bad practice in the first place but anyway xD

So I am thinking, now that we already have a few BC, we should remove this mutation and just generate copies, just as I did 2019 and if people complain, they should change their code.

What do you think?

[anthonyblond]

Interesting, didn't realise the same issue occurred for the data prop too. That seems particularly bad. It didn't occur to me that people would be relying on it...It seems like it would be a a breaking change for the data, but I'm not sure if it would be for columns prop. Its just definitions being passed in, not a 'source of data truth'. I.e I think the data mutation should be treated separately since it might cause more problems.

We got around the problem with the simply enough because we already have a parent component we are using that contains the MaterialTable, so we just use a shallow copy of the each column:

// We don't need to worry anything deeper than the elements themselves...I hope
const columnsCopy = columns.map((x) => ({ ...x }));

[oze4]

@Domino987 should we make those changes you made (but had to remove) and just publish as a new major version? I agree with you that this issue is kind of severe.

If there is a better way to handle it as to not break existing code, I'm all ears. I don't believe npm will automatically upgrade major version. New install is a diff story though.

[peterkmurphy]

This is a four month old fork of an existing library, and probably a good opportunity not to worry so much about "breaking changes".

"if people complain, they should change their code." Damn straight.

[oze4]

I respect that. I feel like putting it in a new major version is meeting in the middle, in a way. But yeah ultimately people will just need to change their code if the "paradigm(s)" we are using make no sense and need to be improved/changed. I've been documenting breaking changes though which helps.

@Domino987 what do you think?

[peterkmurphy]

A quick question: is there a test suite for the library using something like jest (or similar)? It would make it easier to detect breaking changes.

[oze4]

Unfortunately there is not. We have it on the list though. I think the issue isn't detection, rather documentation. If someone switches from mbrn/material-table to his repo, having a place to read a list of breaking changes is useful. We already have one going on the website so it's no biggy.

[Domino987]

Ok so I would suggest to remove the mutation, write in on the website that the object reference is not longer guaranteed and publish a v3 version? @oze4 What do you think?

[Domino987]

This is removed with 3.x