Skip to content
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

Merged
merged 17 commits into from Apr 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "ban usage of React.useLayoutEffect as it produces warnings during SSR",
"packageName": "@fluentui/eslint-plugin",
"email": "olfedias@microsoft.com",
"dependentChangeType": "patch"
}
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "disable lint for valid usages",
"packageName": "@fluentui/react",
"email": "olfedias@microsoft.com",
"dependentChangeType": "none"
}
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "disable lint for valid usages",
"packageName": "@fluentui/react-examples",
"email": "olfedias@microsoft.com",
"dependentChangeType": "none"
}
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "disable lint rules",
"packageName": "@fluentui/react-hooks",
"email": "olfedias@microsoft.com",
"dependentChangeType": "none"
}
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "disable lint rules",
"packageName": "@fluentui/react-tabs",
"email": "olfedias@microsoft.com",
"dependentChangeType": "none"
}
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "use useIsomorphicLayoutEffect to remove a warning during SSR",
"packageName": "@fluentui/react-tooltip",
"email": "olfedias@microsoft.com",
"dependentChangeType": "patch"
}
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "disable lint rules",
"packageName": "@fluentui/react-utilities",
"email": "olfedias@microsoft.com",
"dependentChangeType": "none"
}
13 changes: 13 additions & 0 deletions packages/eslint-plugin/src/configs/react.js
Expand Up @@ -95,10 +95,23 @@ const config = {
message: `"${name}" refers to a DOM global. Did you mean to reference a local value instead?`,
})),
],
'@fluentui/ban-imports': [
'error',
{
path: 'react',
names: ['useLayoutEffect'],
message: '`useLayoutEffect` causes a warning in SSR. Use `useIsomorphicLayoutEffect`',
},
],
'no-restricted-properties': [
'error',
{ object: 'describe', property: 'only', message: 'describe.only should only be used during test development' },
{ object: 'it', property: 'only', message: 'it.only should only be used during test development' },
{
object: 'React',
property: 'useLayoutEffect',
message: '`useLayoutEffect` causes a warning in SSR. Use `useIsomorphicLayoutEffect`',
},
],
'no-shadow': ['error', { hoist: 'all' }],
'no-var': 'error',
Expand Down
Expand Up @@ -29,6 +29,7 @@ const TooltipExampleCore = () => {
{ tooltip: React.useRef<TooltipImperativeHandle>(null), target: React.useRef<HTMLDivElement>(null) },
];

// eslint-disable-next-line no-restricted-properties
React.useLayoutEffect(() => {
refs.forEach(ref => {
if (ref.tooltip.current && ref.target.current) {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-hooks/README.md
Expand Up @@ -208,7 +208,7 @@ const MyComponent = () => {
});
```

## useMountSync
## useMountSync (deprecated)

```ts
const useMountSync: (callback: () => void) => void;
Expand Down
2 changes: 1 addition & 1 deletion packages/react-hooks/etc/react-hooks.api.md
Expand Up @@ -75,7 +75,7 @@ export function useMergedRefs<T>(...refs: (React.Ref<T> | undefined)[]): RefObje
// @public
export const useMount: (callback: () => void) => void;

// @public
// @public @deprecated
export const useMountSync: (callback: () => void) => void;

// @public
Expand Down
1 change: 1 addition & 0 deletions packages/react-hooks/src/useMountSync.test.tsx
Expand Up @@ -7,6 +7,7 @@ describe('useMountSync', () => {
const onMount = jest.fn();

const TestComponent: React.FunctionComponent = () => {
// eslint-disable-next-line deprecation/deprecation
useMountSync(() => {
onMount();
});
Expand Down
5 changes: 4 additions & 1 deletion packages/react-hooks/src/useMountSync.ts
@@ -1,17 +1,20 @@
import * as React from 'react';

/**
*Hook which synchronously executes a callback once the component has been mounted.
* Hook which synchronously executes a callback once the component has been mounted.
*
* `WARNING` This should only be used if you need to perform an action after the component has been mounted and
* before the browser paints. useMountSync will trigger debug warnings in server-rendered scenarios and should be used
* sparingly.
*
* @deprecated Consider to use React.useEffect() or React.useLayoutEffect() directly based on a use case
*
* @param callback - Function to call once the component has been mounted.
*/
export const useMountSync = (callback: () => void) => {
Copy link
Member

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.

Copy link
Member Author

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 👍

const mountRef = React.useRef(callback);
mountRef.current = callback;
// eslint-disable-next-line no-restricted-properties
React.useLayoutEffect(() => {
mountRef.current?.();
}, []);
Expand Down
1 change: 1 addition & 0 deletions packages/react-tabs/src/utilities/useOverflow.ts
Expand Up @@ -75,6 +75,7 @@ export const useOverflow = ({ onOverflowItemsChanged, rtl, pinnedIndex }: Overfl
return () => containerRef(null);
});

// eslint-disable-next-line no-restricted-properties
Copy link
Member

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)

Copy link
Contributor

@behowell behowell Apr 21, 2021

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?

Copy link
Member

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.

Copy link
Member Author

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

React.useLayoutEffect(() => {
const container = containerRef.current;
const menuButton = menuButtonRef.current;
Expand Down
Expand Up @@ -3,6 +3,7 @@ import { makeMergeProps, useMergedRefs } from '@fluentui/react-utilities';
import { TooltipProviderProps, TooltipProviderState } from './TooltipProvider.types';
import { useTooltipManager } from './useTooltipManager';
import { useFluent } from '@fluentui/react-shared-contexts';
import { useIsomorphicLayoutEffect } from '@fluentui/react-utilities';

const mergeProps = makeMergeProps<TooltipProviderState>();

Expand Down Expand Up @@ -45,7 +46,7 @@ export const useTooltipProvider = (
props,
);

React.useLayoutEffect(() => {
useIsomorphicLayoutEffect(() => {
const root = state.ref.current;
if (root && tooltipContainer) {
root.appendChild(tooltipContainer);
Expand Down
1 change: 1 addition & 0 deletions packages/react-utilities/src/descendants/descendants.tsx
Expand Up @@ -73,6 +73,7 @@ export function useDescendant<DescendantType extends Descendant>(
});

// Prevent any flashing
// eslint-disable-next-line no-restricted-properties
React.useLayoutEffect(() => {
if (!descendant.element) {
forceUpdate();
Expand Down
Expand Up @@ -47,6 +47,7 @@ describe('useEventCallback', () => {
// Arrange
const useTestHook = () => {
const callback = useEventCallback(jest.fn());
// eslint-disable-next-line no-restricted-properties
React.useLayoutEffect(() => callback(), [callback]);
};

Expand Down
Expand Up @@ -10,4 +10,5 @@ import { isSSR } from '../utils/index';
* https://gist.github.com/gaearon/e7d97cdf38a2907924ea12e4ebdf3c85
* https://github.com/reduxjs/react-redux/blob/master/src/utils/useIsomorphicLayoutEffect.js
*/
// eslint-disable-next-line no-restricted-properties
export const useIsomorphicLayoutEffect: typeof React.useEffect = isSSR() ? React.useEffect : React.useLayoutEffect;
1 change: 1 addition & 0 deletions packages/react/src/components/Image/Image.base.tsx
Expand Up @@ -24,6 +24,7 @@ function useLoadState(

const [loadState, setLoadState] = React.useState<ImageLoadState>(ImageLoadState.notLoaded);

// eslint-disable-next-line no-restricted-properties
React.useLayoutEffect(() => {
// If the src property changes, reset the load state
// (does nothing if the load state is already notLoaded)
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/components/KeytipData/useKeytipData.ts
Expand Up @@ -26,6 +26,7 @@ export function useKeytipData(options: KeytipDataOptions): IKeytipData {
const prevOptions = usePrevious(options);

// useLayoutEffect used to strictly emulate didUpdate/didMount behavior
// eslint-disable-next-line no-restricted-properties
React.useLayoutEffect(() => {
if (
uniqueId.current &&
Expand All @@ -36,6 +37,7 @@ export function useKeytipData(options: KeytipDataOptions): IKeytipData {
}
});

// eslint-disable-next-line no-restricted-properties
React.useLayoutEffect(() => {
// Register Keytip in KeytipManager
if (keytipProps) {
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/components/Layer/Layer.base.tsx
Expand Up @@ -93,6 +93,7 @@ export const LayerBase: React.FunctionComponent<ILayerProps> = React.forwardRef<
onLayerDidMount?.();
};

// eslint-disable-next-line no-restricted-properties
React.useLayoutEffect(() => {
createLayerElement();
// Check if the user provided a hostId prop and register the layer with the ID.
Expand Down
Expand Up @@ -344,6 +344,7 @@ export const MaskedTextField: React.FunctionComponent<IMaskedTextFieldProps> = R
}, [mask, value]);

// Run before browser paint to avoid flickering from selection reset.
// eslint-disable-next-line no-restricted-properties
React.useLayoutEffect(() => {
// Move the cursor to position before paint.
if (maskCursorPosition !== undefined && textField.current) {
Expand Down
1 change: 1 addition & 0 deletions packages/react/src/utilities/useOverflow.ts
Expand Up @@ -75,6 +75,7 @@ export const useOverflow = ({ onOverflowItemsChanged, rtl, pinnedIndex }: Overfl
return () => containerRef(null);
});

// eslint-disable-next-line no-restricted-properties
React.useLayoutEffect(() => {
const container = containerRef.current;
const menuButton = menuButtonRef.current;
Expand Down