use useSyncExternalStore to subscribe to ResizeObserver#3873
use useSyncExternalStore to subscribe to ResizeObserver#3873amanmahajan7 wants to merge 5 commits intomainfrom
useSyncExternalStore to subscribe to ResizeObserver#3873Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3873 +/- ##
=======================================
Coverage 97.84% 97.84%
=======================================
Files 42 42
Lines 1253 1254 +1
Branches 348 347 -1
=======================================
+ Hits 1226 1227 +1
Misses 27 27
🚀 New features to boost your workflow:
|
amanmahajan7
changed the title
useSyncExternalStore to subsrcibe to ResizeObserveruseSyncExternalStore to subscribe to ResizeObserver
src/hooks/useGridDimensions.ts
Outdated
|
|
||
| const { clientWidth, clientHeight, offsetWidth, offsetHeight } = gridRef.current!; | ||
| const subscribe = useCallback((callback: () => void) => { | ||
| const element = gridRef.current!; |
There was a problem hiding this comment.
How do we know .current is not nullish here?
There was a problem hiding this comment.
This is the place to setup subscription so I assumed .current is not null. I can double check
There was a problem hiding this comment.
subscribe is called after useLayoutEffect
src/hooks/useGridDimensions.ts
Outdated
| }, []); | ||
|
|
||
| const getSnapShot = useCallback(() => { | ||
| return dimensionsRef.current; |
There was a problem hiding this comment.
Don't we read .current during render since we call getSnapShot() to intialize the useSyncExternalStore?
There was a problem hiding this comment.
Not sure 🤔 react-complier did not complain. I can add a class
d13b83f#diff-2dbc2c9db9fe9009d9fe02a7a37f929629e684c70a2d02fff405bb30499ccb24R11
|
|
||
| useLayoutEffect(() => { | ||
| const { ResizeObserver } = window; | ||
| const defaultDimensions: Dimensions = [1, 1, 0]; |
There was a problem hiding this comment.
Wonder if we should change the default to a higher value and get rid of initial width/height logic in the subscribe function
| }); | ||
| const { clientHeight, offsetHeight } = element; | ||
| this.#dimensions = [size.inlineSize, size.blockSize, offsetHeight - clientHeight]; | ||
| callback(); |
There was a problem hiding this comment.
Should we avoid calling callback() if all three values have not changed?
Overall I feel like this is might not be a good fit, the layout effect seems simpler to me.
There was a problem hiding this comment.
Yeah, I am on the fence too leaning towards keeping useSyncExternalStore
Pros
flushSyncis no longer neededgetServerSnapshothandles server side rendering so we do not need to checkif (ResizeObserver == null);- Seems like
useSyncExternalStoreis designed for this specific use case
Cons
- Seems a bit more complex
- Not entirely sure if there can be issues mixing
refanduseSyncExternalStore - Need to manually check values to avoid re-render but I don't know if resizer observer is fired if the dimensions have not changed.
There was a problem hiding this comment.
I will close this PR for now. We can revisit if needed
3 participants
https://www.epicreact.dev/use-sync-external-store-demystified-for-practical-react-development-w5ac0