Add SSR support - #70
Conversation
treycucco
left a comment
treycucco
left a comment
There was a problem hiding this comment.
This looks like a good change Emma. I want to have Vitor look over it since I haven't done anything in this repo. My main suggestions, listed in the comments below is this:
Don't unconditionally call set state, use the cached value being defined to bail out. Default the state to use the cached value, or fallback if necessary. And wrap all that logic (the state and the effect) into a named hook that only returns the width, so all the hooks dealing with reading and calculating the width are batched up together.
| useLayoutEffect(() => { | ||
| setOSScrollbarWidth(getOSScrollbarWidth()); | ||
| }, []); |
There was a problem hiding this comment.
So the idea here is that the useLayoutEffect won't be run by SSR by default, only live, so we avoid the document reference in the IIFE. And then components that need it will usually get the cached value, and the first one will use a layout effect so we won't get any flashing.
One change I'd make: add a ref that is set to true once getOSSScrollbarWidth() has been run so we're not calling a state setter for every layout. It shouldn't matter if the value doesn't change, but it makes me nervous to see an unconditional setState in a useLayoutEffect
| const [scrollRatio, setScrollRatio] = useState(1); | ||
| const [isDraggingTrack, setIsDraggingTrack] = useState(false); | ||
| const [osScrollbarWidth, setOSScrollbarWidth] = useState( | ||
| DEFAULT_SCROLLBAR_WIDTH, |
There was a problem hiding this comment.
Could this be useState(cachedOSScrollbarWidth || DEFAULT_SCROLLBAR_WIDTH) and then add a const calculateWidthRef = useRef(cachedOSScrollbarWidth===undefined)? Then the useLayoutEffect could check that calculateWidthRef and bail out if it is set. Or I guess it could just bail out if the cached scrollbar width is not defined, since the ref is merely tracking that, so no ref needed.
And actually, I think I'd take this useState and the useLayoutEffect and pull them out to their own named hook like useScrollbarWidth() so all that logic is bound up together. That would clean up nicely.
buzinas
left a comment
buzinas
left a comment
There was a problem hiding this comment.
Overall looks good, but I'm wondering if we could avoid the effect by just letting the calculation happens as it did before.
| width: `calc(100% + ${OS_SCROLLBAR_WIDTH}px)`, | ||
| right: `-${scrollbarWidth}px`, | ||
| padding: `0 ${scrollbarWidth}px 0 0`, | ||
| width: `calc(100% + ${osScrollbarWidth}px)`, |
There was a problem hiding this comment.
Any reason why this is using osScrollbarWidth instead of scrollbarWidth like the others?
| width: `calc(100% + ${osScrollbarWidth}px)`, | |
| width: `calc(100% + ${scrollbarWidth}px)`, |
| /** Measures once, cached for every instance; skips re-measuring on remount. */ | ||
| function useOSScrollbarWidth(): number { | ||
| const [osScrollbarWidth, setOSScrollbarWidth] = useState( | ||
| cachedOSScrollbarWidth ?? DEFAULT_SCROLLBAR_WIDTH, | ||
| ); | ||
|
|
||
| useLayoutEffect(() => { | ||
| if (cachedOSScrollbarWidth !== undefined) return; | ||
|
|
||
| cachedOSScrollbarWidth = measureOSScrollbarWidth(); | ||
| setOSScrollbarWidth(cachedOSScrollbarWidth); | ||
| }, []); | ||
|
|
||
| return osScrollbarWidth; | ||
| } |
There was a problem hiding this comment.
Any thoughts on just doing the same as before but gating it? Something like:
const OS_SCROLLBAR_WIDTH = typeof window !== 'undefined' ? oldCode : 20;3 participants
This moves the scrollbar width measurement into an effect so that it won't run during SSR.
To test this out, I linked my locally built
react-custom-scrollerto a repo that has prerendering enabled, and verified that there were no errors while running or building.