useResizeableDrawer: remove size state storage #88170
Conversation
github-actions
bot
added
the
Scope: Frontend
| @@ -1,5 +1,5 @@ | |||
| import type {RefObject} from 'react'; | |||
There was a problem hiding this comment.
@ryan953 would you mind linking me to where this component is in the UI? I have verified that this works in profiling, but I would like to check for replay as well
There was a problem hiding this comment.
oh, sorry for the huge delay!
this is imported by the replay network details thing. So if you're looking at a replay, click into the Network tab, then click on a specific network row.... you'll get Network Details coming up from the bottom. It's resizable.
There was a problem hiding this comment.
Thanks @ryan953, not a problem about the delay. I have more stuff that I'll need to fix, I just opened a bunch of stacked PRs to get this in, and I'll make sure to test everything where this is used before merging this in
| direction: 'up', | ||
| initialSize, | ||
| min: 0, | ||
| onResize: () => {}, | ||
| onResize: (newSize: number) => { |
There was a problem hiding this comment.
types can be inferred:
| onResize: (newSize: number) => { | |
| onResize: (newSize) => { |
| } = useResizableDrawer({ | ||
| direction: isLeftRight ? 'left' : 'down', | ||
| initialSize, | ||
| min, | ||
| onResize: onResize ?? (() => {}), | ||
| onResize: (size: number) => { |
There was a problem hiding this comment.
| onResize: (size: number) => { | |
| onResize: (size) => { |
| localStorage.setItem(options.sizeStorageKey, newSize.toString()); | ||
| } | ||
| sizeRef.current = newSize; | ||
| options.onResize(newSize, userEvent); | ||
| }, | ||
| [options] |
There was a problem hiding this comment.
making a memoized version of something with useCallback that depends on an object passed in as a prop (options) is rarely a good idea, because it means consumers would need to memoize the options object, which isn’t clear, so lots of usages don’t do it. It’s also not a great API.
If we call useResizableDrawer with an inline object, this useCallback does nothing. That also means all further memoization that relies on updateSize will be disappointed because it will be a new reference every render.
I guess the idea here was to have memoized versions returned to consumers, and there are consumers that do it right, like flamegraphDrawer/profileDetails.tsx, where we actually pass the returned functions to a memo’d component.
My suggestion here would be to use the latest ref pattern to write the options to a ref, then always use the latest version of it inside useCallback. This ensures consumers can pass options in however they want:
const optionsRef = useRef(options);
useLayoutEffect(() => {
optionsRef.current = options;
});
This is also what react-query does to the options, because we need them in a useEffect, but don’t want it to be “reactive” towards it:
This is also where useEffectEvent from React itself will help in the future.
There was a problem hiding this comment.
Agree. This API is bad and needs to be changed.
Curious, but why even use a useLayoutEffect for the latest ref? Couldn't you just write to the ref inside the hook body directly, I don't think that changes anything + you can drop the useLayoutEffect. The hook will be invoked if the options object changes, meaning you'd still get the latest value each time
const optionsRef = useRef(options)
optionsRef.current = options;
There was a problem hiding this comment.
it’s technically not allowed to write to refs during render, as that is seen as a side-effect. The react compiler won’t optimize components were it sees those patterns. Technically, react could start a render, you would write to the ref, then it aborts a render (e.g. when using concurrent features). In those cases, the ref would’ve gotten updated “too early”, as the render didn’t complete.
so, it’s just against the “rules of react”.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [options.direction]); |
There was a problem hiding this comment.
the only thing we exclude here is options, but if options were put into a ref with the pattern I outlined before, we wouldn’t need to disable the linter here.
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #88170 +/- ##
==========================================
+ Coverage 85.40% 87.71% +2.30%
==========================================
Files 10180 9977 -203
Lines 583567 563533 -20034
Branches 22640 22210 -430
==========================================
- Hits 498406 494281 -4125
+ Misses 84709 68836 -15873
+ Partials 452 416 -36 |
I am slowly going to remove functionality and stateful behavior from this hook, as it should have never been added on the first place. We want the least opinionated version of a resizeable drawer so that it can cater to different behaviors.