Memory leak with useMediaQuery hook

[JosefMackuAmn]

On swithing page from Home to Site and back, there are few objects leaking, e.g. Mql or MediaQueryList (as can be seen from comparing memory snapshots).

I am using react-router and minimal app is attached.

[chozzz]

@JosefMackuAmn Please post your code instead of the screenshot in the future.

TL:DR; EDITED
The smell comes from this matchmediaquery's update function - Which can be reproduced as following;

• Put a logger inside the update function
• Switch pages for N times
• Trigger the update function by resizing the window. You'll notice your logger will be printed out N times.

For some reason, calling dispose and replacing it with addEventListener('change', update) and removeEventListener('change', update) didn't seem to help.

---

For those who want to reproduce, here's the code;

import React from 'react';
import ReactDOM from 'react-dom';
import { useMediaQuery } from "react-responsive";
import { Switch, Route, NavLink, BrowserRouter as Router } from "react-router-dom";

const Home = () => {
  return (
    <div>
      <h1>Home</h1>
      <NavLink to="/site">Site</NavLink>
    </div>
  );
};

const Site = () => {
  const mq = useMediaQuery({ maxWidth: '1400px' });

  return (
    <div>
      <h1>Site</h1>
      <NavLink to="/">Home</NavLink>
      {
        mq && <div>Under 1400px</div>
      }
    </div>
  );
};

ReactDOM.render(
  <React.StrictMode>
    <Router>
      <Switch>
        <Route path="/" exact component={Home} />
        <Route path="/site" exact component={Site} />
      </Switch>
    </Router>
  </React.StrictMode>,
  document.getElementById('root')
);

And the leak based on switching page back and forth 6 times

[yocontra]

@chozzz Thanks for the detailed write-up! Based on what you found it seems like the issue should be resolved in matchmediaquery - removeListener should be switched to removeEventListener. It should be a pretty simple change to test out (manually edit the package on your local fs), see if it fixes it for you, and PR to that repo.

Based on the docs you linked I would think that addListener should also be replaced with addEventListener for this to work correctly.

[chozzz]

@contra For some reason, I had suspected the wrong code and I am not sure how I ended up there at removeListener.
I re-confirmed removeListener or removeEventListener('change', ...) both remove the events in this case.

However, I have edited the investigation above and the suspect seems to be coming from matchmediaquery update function listener - Which if this code is commented, the leak is gone.

[chozzz]

So escalating from above, which I suppose it was on the right track. Basically, the fact is matchmediaquery update function - Which can only be cleaned via its dispose method.

Proposed Fix

So, based on above, the dispose method needs to be called once we finish using Mql

@contra I have added a PR here - please review #281

---

@JosefMackuAmn As a side note, if you run with CRA in development mode, you might still see the leak as when the first time the component loads, react-dom.development.js may render your <Site> twice. Which the first call to the component never get unmounted.

I learned this the hard way when isolating this issue, and they're evil enough to actually disable console.log on the 2nd render.