useGridDimensions: useSyncExternalStore implementation#3968
useGridDimensions: useSyncExternalStore implementation#3968
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3968 +/- ##
==========================================
+ Coverage 97.53% 97.58% +0.04%
==========================================
Files 36 36
Lines 1463 1492 +29
Branches 472 478 +6
==========================================
+ Hits 1427 1456 +29
Misses 36 36
🚀 New features to boost your workflow:
|
| params[0] instanceof Error && | ||
| params[0].message === 'ResizeObserver loop completed with undelivered notifications.' | ||
| ) { | ||
| return; |
There was a problem hiding this comment.
Just hoping this will be resolved 🤞
| // don't break in Node.js (SSR), jsdom, and environments that don't support ResizeObserver | ||
| const resizeObserver = | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| globalThis.ResizeObserver == null ? null : new ResizeObserver(resizeObserverCallback); |
There was a problem hiding this comment.
We'll have 1 RO instance for all rendered grids on the page, so all resize events will be batched together -> improved perf?
| const idToTargetMap = new Map<string, HTMLDivElement>(); | ||
| // use an unmanaged WeakMap so we preserve the cache even when | ||
| // the component partially unmounts via Suspense or Activity | ||
| const sizeMap = new WeakMap<HTMLDivElement, ResizeObserverSize>(); |
There was a problem hiding this comment.
I took great care to ensure this works great even if the components unmounts but the div isn't deleted.
The updated hook is more complicated than before, but should behave great with concurrent rendering ✌️
| } | ||
|
|
||
| export function useGridDimensions() { | ||
| const id = useId(); |
There was a problem hiding this comment.
Using an id to sync things.
I tried using gridRef.current at first, but getSnapshot gets called during render, so it would probably not be safe.
| } | ||
| } | ||
| return initialSize; | ||
| }, [id]); |
There was a problem hiding this comment.
getSnapshot gets called multiple times during render, so it has to be as fast as possible, and must avoid returning different values.
amanmahajan7
left a comment
amanmahajan7
left a comment
There was a problem hiding this comment.
I wonder if it is worth using a single RO. Most of the pages should have 1 grid and it would be nice if everything is contained. I am just little concerned about odd bugs because of the new approach. We can go ahead and review if you prefer a single RO
|
The hope is that a single RO will help React batch multiple updates into a single render, while it's unclear if that would be the case with multiple ROs. A good example would be the Master Detail demo page We can also lazily initialize it if you want ( |
nstepien
force-pushed
the
externaldimensions
branch
from
20434d1 to
4b7952b
Compare
3 participants
useSyncExternalStoreseems like the better choice to handle this, especially when concurrent rendering is involved.