React errors in DEV mode

[kostia1st]

I believe there's a problem with this code:

export const createNotifier =
  (
    onResize: Props['onResize'],
    setSize: React.Dispatch<React.SetStateAction<ReactResizeDetectorDimensions>>,
    handleWidth: boolean,
    handleHeight: boolean
  ) =>
  ({ width, height }: ReactResizeDetectorDimensions): void => {
    setSize(prev => {
      if (prev.width === width && prev.height === height) {
        // skip if dimensions haven't changed
        return prev;
      }

      if ((prev.width === width && !handleHeight) || (prev.height === height && !handleWidth)) {
        // process `handleHeight/handleWidth` props
        return prev;
      }

      onResize?.(width, height);
      // ^^^^^^^^^^^^^^

      return { width, height };
    });
  };

Calling onResize inside of setState (of the component where the useResizer hook is used) is producing a warning in case the onResize handler in turn modifies internal state of another (for example parent's) component.

In my case React throws this message into console:

Warning: Cannot update a component (Popup) while rendering a different component (PopupBase). To locate the bad setState() call inside PopupBase, follow the stack trace as described in https://reactjs.org/link/setstate-in-render

I guess, simply moving this event call to some kind of useEffect, or using Promise.resolve().then(() => onResize?.(width, height)) or setTimeout(...) would mitigate the issue. The idea is to make the call from a different event.

[tylerprice1]

I agree with this. We should be calling onResize inside the useEnhancedEffect callback, not during the setSize callback. This callback runs during the next render and should be pure.

I don't think you'll run into an issue with using size directly for the previous value when determining whether to call setSize, because there shouldn't be more than one entry in the ResizeObserver callback at any time, so setSize should only be called once per change.

export const createNotifier =
  (
    onResize: Props['onResize'],
    size: ReactResizeDetectorDimensions,
    setSize: React.Dispatch<React.SetStateAction<ReactResizeDetectorDimensions>>,
    handleWidth: boolean,
    handleHeight: boolean
  ) =>
  ({ width, height }: ReactResizeDetectorDimensions): void => {
      if (size.width === width && size.height === height) {
        // skip if dimensions haven't changed
        return;
      }

      if ((size.width === width && !handleHeight) || (size.height === height && !handleWidth)) {
        // process `handleHeight/handleWidth` props
        return;
      }

      onResize?.(width, height);
      setSize({ width, height });
  };

Then in the useEnhancedEffect callback:

const notifyResize = createNotifier(onResize, size, setSize, handleWidth, handleHeight);

[tylerprice1]

@kostia1st I created a PR for this issue 😁 #239. Hopefully we can get it merged

[maslianok]

@kostia1st Thank you for such a detailed issue
and thank you @tylerprice1 for the investigation and the time you put into fixing the issue!

As @kostia1st suggested I simply moved "onResize" to "useEffect" 8730195#diff-045dde5368069c113c4ac044407e3cba3f7cc3db1c6f0e166514b1cc9f5cf28fR72-R74

This should do the trick.
Published in v8.1

[tylerprice1]

@maslianok Thanks so much! My only concern with that approach is that the onResize call will happen after the next render, so the onResize will be called a render late, and if they do something in their callback that triggers another render, that's one extra render than before, which may* hurt performance. React documentation link: https://react.dev/learn/you-might-not-need-an-effect#notifying-parent-components-about-state-changes.

*I've not done any performance tests, but since the ResizeObserver can trigger so frequently, I can imagine a noticeable impact to performance. But the nature of performance is not to bother optimizing too much unless you are noticing a problem or have numbers to back it up, so that may be a moot point.

[maslianok]

@tylerprice1 Good point. Actually I think your approach is better 👍

I resolved conflicts in the PR and left one comment.

[maslianok]

@tylerprice1 Unfortunately, your approach has some pitfalls.

Imagine we have the next use case: component subscribes to onResize events to change some state.

const [count, setCount] = useState(0);

const { width, height, ref } = useResizeDetector({
    onResize: () => {
      console.log(count);
      setCount(count + 1);
    }
  });

What actually happens (step-by-step):

• ResizeObserver triggers resize event
• the library calls the 'onResize' callback
• this callback changes the components state
• component gets updated
• useResizeDetector gets executed with onResize: () => {...}
• Important This inline function is not equal to the previous onResize function from the js perspective, so the library has to setup a new Observer with a new callback in order to call this new onResize function inside the callback. Simply said, we have to re-execute the whole useEffect on every component update

[maslianok]

Let me add, that the previous version had the same problems. But the current one, based on a separate useEffect for the "onResize" event triggers the ResizeObserver initialization only when props change.

[tylerprice1]

You're right. We can do a similar thing to what I did in my PR with sizeRef here with onResize.

const onResizeRef = useRef(onResize);
onResizeRef.current = onResize;

Then we can just pass the ref around so that the effect always has the updated value without having to recreate the ResizeObserver. Although your current advice to wrap the onResize parameter in useCallback also works well

[maslianok]

You're right. We can do a similar thing to what I did in my PR with sizeRef here with onResize.

Oh, that's interesting! Do you see any downsides of this approach?

Do you think it's better to call onResize before or after setSize?

  onResizeRef.current?.(width, height);
  setSize(newSize);
// or
  setSize(newSize);
  onResizeRef.current?.(width, height);

Won't we have problems with the following code snippet?

const { width,height } = useResizeDetector({ onResize });

const onResize = () => {
  setSomeParentState(width,height); // <---- old width & height
}

[tylerprice1]

Downsides

To me, the only real downside to using refs like this is it may be confusing if you don't know what's going on, but I don't think that'll be an issue here, and I tried to document sizeRef well for that reason. React* discourages doing this when possible, but there's no good way to do it currently. Regarding their documentation:

• useReducer will have the same dev error issue (the one specified in this ticket) as useState, because useState is really just a special case of useReducer.
• useRef works how we want but isn't exactly encouraged by React. But in cases like this where performance is going to be impacted, I think useRef is the right way to go. Adding a custom hook to hold the logic can help keep things neater. React's legacy docs* describe it as a way to have "something like this in a class" in a function component.

*They mention using Refs for this in their legacy documentation (I can't find it in their new documentation) https://legacy.reactjs.org/docs/hooks-faq.html#what-can-i-do-if-my-effect-dependencies-change-too-often

Something like this in a utils file:

function useRefToReactiveValue<T>(value: T): MutableRefObject<T> {
    const ref = useRef<T>(value);
    ref.current = value;
    return ref;
}

Then in useResizeDetector.ts replace my code

const sizeRef = useRef(size);
sizeRef.current = size;

with

const sizeRef = useRefToReactiveValue(size);

And we can add:

const onResizeRef = useRefToReactiveValue(onResize);

---

Ordering

I don't think the ordering would matter at all, but I'd probably set my own state before notifying the consumer like

setSize(newSize);
onResizeRef.current?.(width, height);

---

onResize problems

For the second part, shouldn't they be using the parameters passed to onResize? Like

const onResize = (width, height) => {
    setSomeParentState(width, height);
};

Then there shouldn't be an issue with stale values.

[tylerprice1]

Another thought I'm having that I wanted to put in a separate comment

I think the "React" way to do this would be to lift the state up. What this would mean here is either:

• Completely remove the size state from the hook/class and rely purely on the onResize parameter
  • The hook would only store its values in a Ref and the class in an instance variable.
  • What I like about this is that the consumer can choose whether to update the value, so they can avoid re-rendering if they don't actually care about the current size of the element (like if the element is currently hidden), but that can just as easily be done in the hook/class with a shouldUpdate prop (or similar name).
• Remove the onResize parameter and have the hook be the only owner of state
  • This has weird implications in the class, because without onResize the only way I can think of to get the values to the consumer is to change the type of children to be a function like children: (width, height) => JSX.Element | null, but this isn't necessarily a weird or uncommon design pattern.
  • What I like about this for the hook is that it gives the really natural code of const { ref, width, height } = useResizeDetector()

The downside of either of these approaches is that it would be a breaking API change

---

An example of how using useResizeDetector without onResize may look:

🚫 Bad code

function Parent() {
    const [size, setSize] = useState({ width: 0, height: 0 });
    return (
         <div>
             ...
             <Child onSizeChange={setSize} />
             ...
         </div>
    );
};

function Child() {
    const { ref: resizeRef, width, height } = useResizeDetector();
    // Adding a useEffect here means that the state is owned by the wrong component
    useEffect(() => {
        onSizeChange({ width, height });
    }, [width, height, onSizeChange]);
    return <div ref={resizeRef}>...</div>;
}

✅ Better code

function Parent() {
    const { ref: resizeRef, width, height } = useResizeDetector();
    return (
         <div>
             ...
             <Child ref={resizeRef} />
             ...
         </div>
    );
};

const Child = forwardRef(function Child(props, ref) {
    return <div ref={ref}>...</div>;
})

If both child and parent want access to the size, the "proper" approach is probably for each of them to have their own instance of useResizeDetector and to merge the refs together... although that's very much debatable, because I'm not 100% confident in that lol.

[maslianok]

@tylerprice1 Thank you for engaging in this discussion! Your comments reveal your profound understanding of the subject.

There is always a dilemma between performance and user-friendliness.

Lifting the state, lifting the ref up, expecting users to wrap functions into "useCallback" hook... None of these seem to work effectively. Most users prefer simplicity and are not concerned about performance.

In situations where your application has been optimized to the extent that an additional re-render becomes a hindrance, you might consider rewriting the code using the native ResizeObserver API. Alternatively, you could create an additional component and encapsulate it within a "memo" function, as follows:

// Before
function Parent() {
  const { ref: resizeRef, width, height } = useResizeDetector();
  return (
    <div ref={ref}>
      <div>
        // ...
      </div>
    </div>
  );
};

// After
const Child = React.memo(() => {
  return (
    <div>
      // ...
    </div>
  );
});

function Parent() {
  const { ref, width, height } = useResizeDetector();
  return <div ref={ref}><Child height={height} /></div>;
};

In essence, my aim is to maintain the library's simplicity and ease of use. This allows new users to seamlessly integrate it into their existing codebases within a short time frame.

I hope it clarifies my perspective on the library and explains why I am not in favor of lifting the state and removing the onResize callback. However, it doesn't prevent me from saying that your suggestions are very good for personal use! 👍