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

Stop mutating column.options props and support immutable data object #997

Merged
merged 2 commits into from
Oct 15, 2019

Conversation

n3tr
Copy link
Contributor

@n3tr n3tr commented Oct 11, 2019

I notice that the MUIDataTable is mutating the props.columns.options value and it causes error when passing an immutable object to it.

Example, I'm using immer to update state and it produces immutable data object (using deep-freeze under the hood).

I think it is better to not mutate the props values inside the component.

Change

  • Replace code that mutate props.columns.options by spreading column.options into new options before mutating

Example
https://codesandbox.io/s/dazzling-dijkstra-kmuf2

@n3tr n3tr changed the title Stop mutating colums props Support immutable object props, stop mutating colums props Oct 11, 2019
@n3tr n3tr changed the title Support immutable object props, stop mutating colums props Stop mutating column.options props and support immutable data object Oct 11, 2019
Copy link
Collaborator

@gabrielliwerant gabrielliwerant left a comment

Choose a reason for hiding this comment

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

Fair enough!

Couple of minor issues.

  • The 'should correctly build internal table data and displayData structure when using nested data' test is broken by the changes, I think because you missed also changing line 425 to options.sortDirection.

  • Line 412 is missing a semicolon which prevents webpack from compiling the project when running it in development.

@coveralls
Copy link

coveralls commented Oct 11, 2019

Coverage Status

Coverage decreased (-0.08%) to 77.279% when pulling c7a9d53 on n3tr:stop-mutating-column-props into b8bee52 on gregnb:master.

@n3tr
Copy link
Contributor Author

n3tr commented Oct 11, 2019

@gabrielliwerant Fixed :)

Sorry, I forgot to run test before push the change and didn't realize it breaks a test case.

@gabrielliwerant
Copy link
Collaborator

gabrielliwerant commented Oct 11, 2019

Looks like you accidentally pushed your local yarn lockfile with the last commit! I noticed when the line changes jumped by about 9k. :)

@n3tr n3tr force-pushed the stop-mutating-column-props branch from 316690f to c7a9d53 Compare October 11, 2019 07:46
@n3tr
Copy link
Contributor Author

n3tr commented Oct 11, 2019

@gabrielliwerant removed 😅

Should we add yarn.lock to .gitignore?

@gabrielliwerant
Copy link
Collaborator

@n3tr Hm, I guess that wouldn't hurt. Could also take it out if we switched to yarn in the future. I'll leave that up to you.

@n3tr
Copy link
Contributor Author

n3tr commented Oct 12, 2019

@gabrielliwerant I will open separate PR for removing yarn.lock.

Do you know why TravisCI doesn't send the build status here?

@gabrielliwerant
Copy link
Collaborator

@n3tr Not sure. I actually don't have access to the CI administration for this library. My guess is that it just wasn't set up.

@gabrielliwerant gabrielliwerant merged commit cedc42f into gregnb:master Oct 15, 2019
@n3tr
Copy link
Contributor Author

n3tr commented Oct 15, 2019

@gabrielliwerant look like TravisCI deprecated Github commit status (https://blog.travis-ci.com/2018-09-27-deprecating-github-commit-status-api-for-github-apps-managed-repositories) and require owner to activate Travis CI app for this repo.

waqasajaz pushed a commit to waqasajaz/mui-datatables that referenced this pull request Oct 31, 2019
…regnb#997)

* Stop mutating colums props

* Fix sortDirection condition and add semi-colon
lalong13 pushed a commit to lalong13/mui-datatables that referenced this pull request Jan 15, 2020
…regnb#997)

* Stop mutating colums props

* Fix sortDirection condition and add semi-colon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants