maslianok / react-resize-detector

A Cross-Browser, Event-based, Element Resize Detection for React
http://maslianok.github.io/react-resize-detector/
MIT License
1.25k stars 91 forks source link

useResizeDetector with targetRef - version 1.3.1 #130

Closed whplh closed 3 years ago

whplh commented 3 years ago

Hi, if have the following problem with the new targetRef: If I use: const {width, height} = useResizeDetector({targetRef: someRef}) width and height are always undefined for my component Whereas if I use: const {width, height} = useResizeDetector({targetRef: someRef, onResize: ()=>{}}) I get the correct dimensions of the someRef component,.... which doesn't really make sense to me. I suspect react-resize-detector is not reacting to ref changes properly. Adding ref.current to the dependency array [refreshMode, refreshRate, refreshOptions, handleWidth, handleHeight, onResize] (which helps in my case) seems to be wrong and "wrong" Any ideas? Cheers,

maslianok commented 3 years ago

Yeah, I see the problem...

onResize is an anonymous function. JS creates a new instance of this function every time the parent component rerenders. And, as a result, it triggers useEffect logic...

maslianok commented 3 years ago

After reading this thread I found that we probably use the wrong hook.

I think we should use useLayoutEffect instead of the useEffect.

Can you please try a new prerelease version v6.4.0-rc.0 with this update and confirm that it resolves your problem?

whplh commented 3 years ago

I tried v6.4.0-rc.0 - didn't work. Looking (and playing around with the code) it's basically the same problem. The useLayoutEffect would never know when the ref changes.

From reading the links above and also the react hook FAQ, I think the way the refs are handled currently, is not how it's supposed to be. There probably shouldn't be any targetRef or any other ref you pass to a hook

To formulate more clearly what basically what would be needed here:

Make sure the code in the useEffect/useLayout section runs again if one of the current dependency changes or if there is a change in the ref Element

whplh commented 3 years ago

So here is a suggestion I came up with: (Be carefull, I used any in Typescript ) useResizeDetector.ts.txt

whplh commented 3 years ago

Ah ok I had a look at the other thread. If the ref/node is need from within onResize, why not add it? ~> onResize(width, height, node) Anyway. Let me know what you think. Cheers.

maslianok commented 3 years ago

Well, before diving deep into your suggestions let me clarify one more time: are you sure v6.4.0-rc.0 doesn't solve the problem you described in the initial issue?

Here is the code snippet that works perfectly in my case https://codesandbox.io/s/holy-rain-jjybk

whplh commented 3 years ago

Ok I will try to replicate the issue with an example on codesandbox.

whplh commented 3 years ago

Ok making the example also helped me understand the problem more clearly. It's not just the new targetRef. It's a more general problem with the useResizeDetector Hook. (Basically it's a bug which affects all components which refs are mutated during runtime of the application For these components {width, height, ref} = useResizeDetector() will also not work as expected)

The most simple example I could think of, to replicate the bug : https://codesandbox.io/s/material-demo-forked-iyng4?file=/demo.js

If you use the code from my suggestion, you will see it'll work =)

Let me know if you have any problems with the example (I haven't use codesandbox for this before)

maslianok commented 3 years ago

Aha, I see...

You're trying to implement the approach Dan mentioned in this comment , right?

Does it change the library API? I mean that you can't pass targetRef anymore, right? Can you provide a code snippet of how it's intended to be used?

whplh commented 3 years ago

Well you probably could make it compatible with targetRef option. ( But i don't really like that, or think it's a good idea =) I made a pr for what i have in mind, so we can see & discuss & edit changes over there

maslianok commented 3 years ago

From my point of view:

we have to trigger useLayoutEffect when ref.current gets changed, but we can't use it as a dependency.

What if we use other props? Let's say


// Delay logic
const [isVisible, setIsVisible] = useState(false);
useEffect(() => {
  const timeout = setTimeout(() => {
    setIsVisible(true);
  }, 1000);

  return () => clearTimeout(timeout);
}, []);

const multipleUpdatesRef = useRef();
const { width, height } = useResizeDetector({ targetRef: ref, handleWidth: isVisible });

Pay attention to handleWidth prop...

maslianok commented 3 years ago

So the general idea is to disable resize-detector library when you know that your ref is empty. And enable it when it's filled with some value...

But it won't work when the ref changes... :(

maslianok commented 3 years ago

Well, actually, we have to pass ref.current as a dependency to useLayoutEffect. It will solve all problems...

whplh commented 3 years ago

Well, actually, we have to pass ref.current as a dependency to useLayoutEffect. It will solve all problems...

Yes for the cases i tested that would work. But this approach is dicouraged by the react team. I'm not sure if adding it to the dependency would always guarantee an update on every ref change. (I'm still trying to understand the concept of refs and ref usage)

whplh commented 3 years ago

So the general idea is to disable resize-detector library when you know that your ref is empty. And enable it when it's filled with some value...

But it won't work when the ref changes... :(

Yes. In the use case I encountered the problem the changes came from an library working heavily with async functions, which triggered a delayed ref update.

Yes if you know where the change is coming from like in the example isVisible you can add it to the hook. But in more complicated scenario this might not be the case or practical

maslianok commented 3 years ago

Ok, the more I read that thread the more I think it's fine to use ref.current as a dependency in our particular case.

This comment says there are 2 problems with refs as a dependency:

Re. the first point: yes, it's is a problem for useEffect, but we use useLayoutEffect. As per the documentation it fires synchronously after all DOM mutations. So it's not a problem for us, right?

Re. the second point: it's true that changing the ref.current doesn't trigger an update. But, basically, it should be clear for everyone that React doesn't observe refs and nothing happens when you simply update the ref. And, finally, this problem is much "better" than not processing refs change at all :)

So I would vote to add ref.current as a dependency to useLayoutEffect.

whplh commented 3 years ago

Ok LGTM, and it's also a simple change in the code. (Thank you for discussing this. It helped me understand the ResizeDetector and React =) Cheers.

mungojam commented 2 years ago

Similar to the above, I found that I was getting an initial delay before the width and height would get assessed. Using something in the current react docs, I was able to fix it by avoiding useRef altogether as they recommend:

    const [wrapperRef, setWrapperRef] = useState<
      MutableRefObject<unknown> | undefined
    >(undefined);

    const refCallback = useCallback((node) => {
      setWrapperRef({ current: node });
    }, []);

    const { width, height } = useResizeDetector({
      targetRef: wrapperRef,
    });

    return 
      <div ref={refCallback}>
    <MyBitsUsingWidthAndHeight />
      </div>

I wonder if you should do similar. I've asked if they can add an equivalent example to the beta docs, because I might be relying on some legacy ref thing here.