Closed kostia1st closed 1 year ago
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);
@kostia1st I created a PR for this issue 😁 #239. Hopefully we can get it merged
@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" https://github.com/maslianok/react-resize-detector/commit/8730195daa9d77b8760d85e1279c5eb793ebf1a5#diff-045dde5368069c113c4ac044407e3cba3f7cc3db1c6f0e166514b1cc9f5cf28fR72-R74
This should do the trick. Published in v8.1
@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.
@tylerprice1 Good point. Actually I think your approach is better 👍
I resolved conflicts in the PR and left one comment.
@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):
onResize: () => {...}
useEffect
on every component updateLet 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.
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
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
}
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);
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
problemsFor 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.
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:
size
state from the hook/class and rely purely on the onResize
parameter
Ref
and the class in an instance variable.shouldUpdate
prop (or similar name).onResize
parameter and have the hook be the only owner of state
children
to be a function like children: (width, height) => JSX.Element | null
, but this isn't necessarily a weird or uncommon design pattern.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.
@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! 👍
I believe there's a problem with this code:
Calling
onResize
inside ofsetState
(of the component where theuseResizer
hook is used) is producing a warning in case theonResize
handler in turn modifies internal state of another (for example parent's) component.In my case React throws this message into console:
I guess, simply moving this event call to some kind of
useEffect
, or usingPromise.resolve().then(() => onResize?.(width, height))
orsetTimeout(...)
would mitigate the issue. The idea is to make the call from a different event.