fix(react-tooltip): use useIsomorphicLayoutEffect to avoid SSR warnings #17894
Conversation
Asset size changes
Baseline commit: 88a432afc115f1750a3bc1afa67a3d90960db903 (build) |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit bfb7f8b:
|
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
…hore/ban-layout-effect
| @@ -12,6 +12,7 @@ import * as React from 'react'; | |||
| export const useMountSync = (callback: () => void) => { | |||
There was a problem hiding this comment.
Can you also mark this as @deprecated? It's not used anywhere and is fundamentally problematic due to useLayoutEffect usage.
There was a problem hiding this comment.
Sure, marked it by JSDoc & in README.md 👍
| @@ -75,6 +75,7 @@ export const useOverflow = ({ onOverflowItemsChanged, rtl, pinnedIndex }: Overfl | |||
| return () => containerRef(null); | |||
| }); | |||
|
|
|||
| // eslint-disable-next-line no-restricted-properties | |||
There was a problem hiding this comment.
@behowell FYI, might be good to look into whether this could safely be updated to use useIsomorphicLayoutEffect, or how to handle SSR properly if that's not possible. (no need for changes in this PR though)
There was a problem hiding this comment.
I'm not familiar enough with SSR to know offhand. Basically this needs to happen at some point--it's ok if it's delayed on the first render after SSR, but it would be bad if it never happened. Is that what useIsomorphicLayoutEffect achieves?
There was a problem hiding this comment.
useIsomorphicLayoutEffect does useLayoutEffect normally, and useEffect if server-rendered. For most things that's okay, but depending on what it's being used for, could cause issues like flashing on first render (or maybe bugs in a few cases that are particularly dependent on the timing).
I'm not sure what a good way is to validate SSR. I'm not sure how to manually test it (would have to look that up). For v8 we have an ssr-tests package, but it's limited to extremely basic verification that the components render.
There was a problem hiding this comment.
Things that are related to DOM calculations/operations probably should never executed on server at all as there is no DOM 🙄
It depends on use case, but sometimes we can simply exclude useLayoutEffect based on environment, for example, it's how it's done in Emotion:
https://github.com/emotion-js/emotion/blob/6c4a9e50f177900f54f7b3368a55b1a7d5e3c002/packages/react/src/global.js#L83-L86
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
|
🎉 Handy links: |
…gs (microsoft#17894) * fix(react-tooltip): use useIsomorphicLayoutEffect to avoid SSR warnings * Change files * fix line length * disable lint for valid usages * Change files * disable lint for valid usages * Change files * disable lint for valid usages * Change files * disable lint for valid usages * Change files * disable lint for valid usages * Change files * mark useMountSync() as deprecated * ignore lint in test
Pull request checklist
$ yarn changeDescription of changes
This PR replaces
React.useLayoutEffectwithuseIsomorphicLayoutEffectas it produces warnings during SSR.Also modifies ESLint preset to prevent future usages. Warnings in existing places of v8 code have been suppressed.
See more details: https://medium.com/@alexandereardon/uselayouteffect-and-ssr-192986cdcf7a