Fix SSR for breadcrumbs and collapsible-nav #3970
Conversation
Closes elastic#3687. Put guards around references to `window` so that in an SSR environment, EUI will still work.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3970/ |
| ); | ||
|
|
||
| const functionToCallOnWindowResize = throttle(() => { | ||
| const newBreakpoint = getBreakpoint(window.innerWidth); | ||
| const newBreakpoint = getBreakpoint( | ||
| typeof window === 'undefined' ? 1024 : window.innerWidth |
There was a problem hiding this comment.
Hmm, I totally understand the intent of this PR, but I don't think setting this to a static (arbitrary) number creates expected behavior.
Instead, is there a way to only do all this breakpoint logic/dependency stuff if the window exists? That way SSR users can apply breakpoints via CSS without it getting overridden by this static number.
There was a problem hiding this comment.
Hmm, not really because you're not supposed to conditionally call hooks like useState. However I could change the code to use e.g. Infinity or something so that getBreakpoint() returns undefined, which it can already do according to its type. What do you think? It's a (somewhat) less arbitrary value?
There was a problem hiding this comment.
I don't think that will actually return undefined because any number that is not less than the largest breakpoint will return the value for the largest breakpoint, ie. xl
There was a problem hiding this comment.
I can swap it for -Infinity, I checked over how values derived from window.innerWidth are used and I think it'll be OK.
(Aside: this is a philosophical brain teaser - what is the size of a server-side window?)
There was a problem hiding this comment.
Accessing window from functionToCallOnWindowResize should never happen in SSR. The method is called from useEffect which doesn't run in a non-browser environment. I believe only the change to currentBreakpoint's initial state is necessary.
There was a problem hiding this comment.
😞 The breakpoint logic also doesn't seem to be working anymore at all (even just on our docs).
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3970/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3970/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3970/ |
|
@pugnascotia Thank you for setting up this branch. I'm trying to get a clear picture how what the behavior of these components will now be. Does this simply mean that for SSR's, on initialization of the component, it won't know what |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3970/ |
|
@cchaos On the server (e.g. Next.js, whether rendering on-demand or pre-rendering for a static site), these changes will cause the breadcrumb component initialse and render (one time only) with a very narrow window width ( Then, the component will re-render on the client (web browser), where it will pick up the window width and render appropriately, and identically as at present. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3970/ |
There was a problem hiding this comment.
Then, the component will re-render on the client (web browser), where it will pick up the window width and render appropriately, and identically as at present.
😅 Ok cool! So nothing changes for the user. The breadcrumbs still properly display depending on the window size at the time of page load and on window resize.
Whew, that means I don't have to continue down my path of writing up an "Except for SSR" callout in the documentation 😸
Co-authored-by: Caroline Horn <[email protected]>
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3970/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3970/ |
Summary
Closes #3687. Put guards around references to
windowso that in an SSR environment, EUI will still work.My choice of 1024 as a default
innerWidthwas entirely arbitrary - I'm open to suggestions here.Checklist