Fix for Memory Leak in TableToolbar

[patorjk]

Fix for #809

See comment #809 (comment) on how to reproduce the problem.

This PR refactors TableToolbar.js to use withStyles instead of styled.js. styled.js was being used because it allows you to access props when setting up your styles. However, the props used were not needed. They were used for this line:

...(props.options.responsive ? { ...responsiveToolbarStyles(theme) } : {}),

According to the docs, options.responsive should have a value of either "scroll" or "stacked", so this check should always resolve to true.

This PR refactors TableToolbar so that it uses withStyles instead of styled.js.

Quick view of PR: https://codesandbox.io/s/mui-datatables-9pmct - You can use the method described in the comment above to see that the memory difference between styled.js and withStyles (original / styled.js version here: https://codesandbox.io/s/muidatatables-custom-toolbar-5o1hr - though you need to enable the Toolbar in that example to see the memory not being released). See #829 (comment)

[patorjk]

Sorry for doing this backwards. I should have verified the memory problem before putting in a pull request. I saw @Fabbok1x comment on styled.js and a comment in the code mentioned that it allowed you to use props within your style rules. This piqued my interest since I thought Material already allowed that (I was mistakenly thinking of makeStyles instead of withStyles). While I was poking around TableToolbar.js I realized styled.js wasn't needed anyway and could be removed. I figured that would just solve the problem, but after thinking about it, unless there was a verifiable problem to solve, the PR wouldn't be that beneficial.

Also, @coveralls was down for maintenance when I put this in. Not sure if there's a way to manually call it.

[patorjk]

As an interesting side note, the memory issue doesn't appear for me on my home computer (iMac with Chrome version 75.0.3770.142). It occurred on both of the Windows boxes I use at work though. I'm not sure of the version of Chrome though, I'll check tomorrow. See my next comment.

[patorjk]

Ugh, ok scratch that. I can produce it.

Here's how to see the memory leak:

• Open: https://codesandbox.io/s/mui-datatables-peujv (this is the current version of the project with a modified customize-filter example).
• Open Chrome Dev Tools and select the Memory tab.
• Select "Allocation instrumentation on timeline"
• Select "codesandbox.io" as the JavaScript VM instance.
• Press "Start"
• Let the table appear/disappear 5-6 times.
• Press "Stop" and look at the chart.

In a separate tab, open up https://codesandbox.io/s/mui-datatables-9pmct and do the exact same steps. Then look at the graph difference between the two sandboxes.

The second sandbox is my PR with the modified TableToolbar.js and a modified customize-filter example.

[gabrielliwerant]

Thank you for your investigative efforts @patorjk! I took your suggested steps to reproduce and I captured each scenario in order:

To me, they don't look appreciably different. I do remember when I tried a few days ago, they did look more dissimilar. I think this is really tricky to trace, especially in codesandbox, because we're dealing with a VM behind the scenes which may be getting its own updates or experiencing its own bugs (and it also seems like more than one being open in a browser at the same time shares resources between them).

So, I couldn't quite locate a real "smoking gun" issue here. However, I think it might be a good idea to keep these changes anyway, as

1. It would help to keep styling implementations unified in the library
2. If there are no new problems caused, it may be worth the attempt in the off chance that it does resolve an issue

What do you think?

[Fabbok1x]

@patorjk : Thank you. All I could add to this is that in my environment I was able to tackle the issue with a 100% certainty on the styled.js HOC beeing one of the core issues of this leak. Removing the HOC will definetly resolve a big part of the leak. To add that React 16.9.0 came up with a fix regarding a major memory leak (facebook/react#16115) aswell. In case you have updated the react version meanwhile this would most probably explain the discrepancy which would/could then definetly seem rather random and might've had its' root cause in the prior mentioned ReactJS leak.In case you have not updated the ReactJS version then a resolution of the leak would still take effect by removing styled.js in a given environment.

[patorjk]

@Fabbok1x Thanks for finding this! This was a pretty amazing find, especially being able to pinpoint the root cause (even if it is unclear why it happens).

@gabrielliwerant I concur, though I've adjusted the timeouts in the examples to make them faster so the issue shows up better over ~20s, however, there could be other factors (browser, OS, etc) that affect it. However, I agree with your assessment on keeping things more unified. That's part of the reason I submitted this without first verifying it, I figured it was good update since worst case it made things a little more simple.

[gabrielliwerant]

Just a bit of history around the hardcoded style HOC, it looks like it was a workaround for pre-3.5 versions of material-ui, but since we are now on 3.5.1, it doesn't seem like there is a need any longer.

[gabrielliwerant]

Nice 👍