Issue #237 - Do not call onResize during setSize updater function in createNotifier #239
Conversation
|
This has the enhancement label, but it's fixing a React error. Should it be labelled a bug instead? |
…ater function in \`createNotifier\`. The updater function must be pure.
tylerprice1
force-pushed
the
Issue237-setSizeSideEffect
branch
from
9d9ac27
to
c87d040
Compare
| @@ -31,22 +31,27 @@ export const isDOMElement = (element: unknown): boolean => | |||
|
|
|||
| export const createNotifier = | |||
| ( | |||
| onResize: Props['onResize'], | |||
| sizeRef: Readonly<React.MutableRefObject<ReactResizeDetectorDimensions>>, | |||
There was a problem hiding this comment.
Do we need this additional variable? Can't we simply compare width and height to the current state?
There was a problem hiding this comment.
I stored the current state inside a ref so that we can access its value within the effect without having to declare it as a dependency and recreate the ResizeObserver every time it changes. It's kind of a hacky approach to avoid stale values or triggering the effect too often
Or did you mean using the setState((prev) => { ... }); pattern?
There was a problem hiding this comment.
I see. That's what you initially did and then refactored to the current solution...
Thank you! I'm going to merge the PR
Updated
createNotifierto conditionally callsetSizebased on the currentsize. Also reference my comment here: #237 (comment)Things work okay in the local example, and I didn't see any unit tests, but I'm happy to do any specific testing that you guys need.