-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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(react-tooltip): use useIsomorphicLayoutEffect to avoid SSR warnings #17894
Conversation
Asset size changes
Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 change
Description of changes
This PR replaces
React.useLayoutEffect
withuseIsomorphicLayoutEffect
as 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