-
-
Notifications
You must be signed in to change notification settings - Fork 105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(breakpoints): races in useViewportWidth #114
fix(breakpoints): races in useViewportWidth #114
Conversation
Hello @agriffis, your code is not compatible with SSR. Maybe the actual strategy is not the good one but it is compatible with SSR. |
ce4e7e5
to
c5e9e05
Compare
I just changed the useState initialization to |
Nop |
@gregberge I can make that change, but just to confirm that we are looking at the same thing... The last commit I offered has var _React$useState = React.useState((_window = window) == null ? void 0 : _window.innerWidth),
width = _React$useState[0],
setWidth = _React$useState[1]; which uses lenient equality, and Please let me know if you would still like me to change it, thanks! |
@gregberge I know you're busy, but I'd love to get this sorted and merged when you get the chance to review my comment above. |
Hello, I was in holiday, but I am ready to merge after you made the change. |
The main fix here is changing the initial state from null to window.innerWidth, so that there isn't a useless transition from (for example) `useDown('md') = true` to `useDown('md') = false` when the application loads. Depending on the application, the rerendering penalty can be heavy. While we're here, notice that we can make a couple related changes: First, since the initial state is set properly, we don't need a layout effect, instead we can use a normal effect. Second, swap the order of listener and setWidth to avoid the tiny race between them.
Sheesh, of course you're right:
|
c5e9e05
to
ca978a7
Compare
@gregberge Should be all set now |
@gregberge You may want to consider reverting this change because it breaks React SSR hydration. When React hydrates, it assumes that the provided HTML matches the initial rendering produced by the client. If there is a mismatch, React won't know that it needs to update the DOM. The virtual DOM and real DOM are out of sync until a change to the virtual DOM triggers a sync. For example: Server: first (only) render, useViewPortWidth returns null, and the component renders Client: first render, useViewPortWidth returns 800, which renders There's another technique that I'm exploring that works like this: const useMyHook = () => {
// ...interesting code that probably depends on window.something...
const [hydrated, setHydrated] = React.useState(false)
React.useEffect(() => {
setHydrated(true)
}, [setHydrated])
return hydrated ? value : null
} This ensures a consistent initial return value between SSR and browser, so that the initial virtual DOM matches the real DOM during hydration. |
Also, I apologize for breaking React SSR hydration. I've learned a lot about it between this PR and now... |
Yeah I know, but there is no good answer here. I think it is preferable to break SSR rehydration in this case to avoid a blink if there is no SSR. We could provide an option to choose the behavior. |
Well, full disclosure, I'm not using xstyled at the moment, because I made some deliberate choices to sacrifice functionality for the sake of fewer dependencies in my current project... However, I have an idea for this: import {useEffect, useLayoutEffect, useState} from 'react'
const useIsomorphicLayoutEffect =
typeof document === 'undefined' ? useEffect : useLayoutEffect
/**
* Return viewport width, updating when the viewport changes. Pass defaultValue
* (not undefined) to enable SSR hydration.
*/
export const useViewportWidth = defaultValue => {
const [width, setWidth] = React.useState(
typeof defaultValue !== 'undefined'
? defaultValue
: typeof window === 'undefined'
? null
: window.innerWidth,
)
useIsomorphicLayoutEffect(() => {
const handleResize = () => setWidth(window.innerWidth)
window.addEventListener('resize', handleResize)
setWidth(window.innerWidth)
return () => window.removeEventListener('resize', handleResize)
}, [setWidth])
return width
} So there are two concepts here:
|
Summary
The main fix here is changing the initial state from null to
window.innerWidth, so that there isn't a useless transition from (for
example)
useDown('md') = true
touseDown('md') = false
when theapplication loads. Depending on the application, the rerendering penalty
can be heavy.
While we're here, notice that we can make a couple related changes:
First, since the initial state is set properly, we don't need a layout
effect, instead we can use a normal effect. Second, swap the order of
listener and setWidth to avoid the tiny race between them.
Test plan
This cropped up for me in storybook. I have a component that measures its
container to make image layout decisions. On the first load of the app, it would
work, but on hot reload, the image would be the wrong size.
The problem is that
useDown('md')
returns the wrong value the first time. Itwas okay on initial app load because image loading slowed things down enough
that
useDown('md')
would return the right value before the measurement.However on hot reload, when the image is already loaded, the measurement would
fire in the window of time between the bogus
useDown
and the rightuseDown
.Note that the measurement code also reruns on resize, but since there's no
resize occurring, it wouldn't know to remeasure.
Making this change fixed the problem for me.