yocontra / react-responsive

CSS media queries in react - for responsive design, and more.
https://contra.io/react-responsive
MIT License
7.04k stars 298 forks source link

Nested responsive components that use refs results in stale elements in Safari #248

Closed shlangus closed 6 months ago

shlangus commented 4 years ago

Hello, we use examples provided in Easy Mode section of the doc and faced the issue that reproduces in Safari (13.0.5) on Mac OS. The problem occurs when the responsive components are nested and there is some child that uses ref. Something like this (sandbox)

const Leaking = ({ children }) => {
  const ref = React.useRef();
  React.useEffect(() => {
    // do smth with ref
    console.log(ref.current.clientWidth);
  });

  return <div ref={ref}>{children}</div>
}

const ResponsiveChild = () => {
  return (
    <div>
      <Desktop>
        <Leaking>Desktop</Leaking>
      </Desktop>
      <Mobile>
        <Leaking>Mobile</Leaking>
      </Mobile>
    </div>
  );
};

export const Issue = () => {
  return (
    <div>
      <Mobile>
        <ResponsiveChild />
      </Mobile>
      <Default>
        <ResponsiveChild />
      </Default>
    </div>
  );

After resizing the window back and forward the following warning occurs:

and some elements get duplicated. We found this issue in 8.0.3 version but it also presents in 7.0.0.

yocontra commented 4 years ago

Thanks for the heads up and providing a reproduction - that should help us fix this quickly.

Ping @Tomekmularczyk since this is related to the hook changes.

shlangus commented 4 years ago

Thank you and let me share some additional info on it. It reproduces with a MediaQuery component as well (I've updated sandbox with it) and presents in 7.0.0 version that does not use hooks. The set and order of components matters, playing with it I was able to get different results. The warning appears regardless of ref usage in a child component but when it is used everything gets broken.

shlangus commented 4 years ago

I managed to fix my case.

  1. Comment this line.
  2. Change useMatches (it is needed because MQ object provided by the library stops updating matches property after step 1).

    const useMatches = (mediaQuery) => {
    const [ matches, setMatches ] = React.useState(mediaQuery.matches)
    
    React.useEffect(() => {
    const updateMatches = ({ matches }) => {
      setMatches(matches)
    }
    mediaQuery.addListener(updateMatches)
    setMatches(mediaQuery.matches);
    
    return () => {
      mediaQuery.removeListener(updateMatches)
    }
    }, [ mediaQuery ])
    
    return matches
    }

    Unfortunately, I cannot explain why it is so. I played with my app and got an infinite updates cycle at some moment, so I guess the event fires a bit differently in Chome and Safari so that mediaQuery.matches used in hook gets a wrong value under some circumstances.

yocontra commented 8 months ago

Is anyone able to reproduce this issue still? Has been 4yrs and I the library has gone through a lot of improvements, this should be fixed. Will close in 30 days if nobody can repro this.