Enable eslint-plugin-react-hooks for react-next (microsoft#14478)

levithomason · Aug 24, 2020 · 4012a18 · 4012a18
1 parent 80e91b1
commit 4012a18
Show file tree

Hide file tree

Showing 26 changed files with 364 additions and 310 deletions.
diff --git a/change/@fluentui-eslint-plugin-2020-08-11-18-34-28-eslint-hooks-next.json b/change/@fluentui-eslint-plugin-2020-08-11-18-34-28-eslint-hooks-next.json
@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "Add rule to prevent accidental references to globals",
+  "packageName": "@fluentui/eslint-plugin",
+  "email": "[email protected]",
+  "dependentChangeType": "patch",
+  "date": "2020-08-12T01:34:12.441Z"
+}
diff --git a/change/@fluentui-react-next-2020-08-11-18-34-28-eslint-hooks-next.json b/change/@fluentui-react-next-2020-08-11-18-34-28-eslint-hooks-next.json
@@ -0,0 +1,8 @@
+{
+  "type": "prerelease",
+  "comment": "Enable eslint-plugin-react-hooks in react-next and fix a few bugs",
+  "packageName": "@fluentui/react-next",
+  "email": "[email protected]",
+  "dependentChangeType": "patch",
+  "date": "2020-08-12T01:34:16.311Z"
+}
diff --git a/change/@uifabric-monaco-editor-2020-08-11-19-02-22-eslint-hooks-next.json b/change/@uifabric-monaco-editor-2020-08-11-19-02-22-eslint-hooks-next.json
@@ -0,0 +1,8 @@
+{
+  "type": "none",
+  "comment": "Fix violation of newly added lint rule",
+  "packageName": "@uifabric/monaco-editor",
+  "email": "[email protected]",
+  "dependentChangeType": "none",
+  "date": "2020-08-12T02:02:22.263Z"
+}
diff --git a/change/@uifabric-test-utilities-2020-08-11-18-34-28-eslint-hooks-next.json b/change/@uifabric-test-utilities-2020-08-11-18-34-28-eslint-hooks-next.json
@@ -0,0 +1,8 @@
+{
+  "type": "none",
+  "comment": "Fix misleading docs for safeMount",
+  "packageName": "@uifabric/test-utilities",
+  "email": "[email protected]",
+  "dependentChangeType": "none",
+  "date": "2020-08-12T01:34:27.992Z"
+}
diff --git a/packages/eslint-plugin/src/configs/react.js b/packages/eslint-plugin/src/configs/react.js
@@ -87,6 +87,13 @@ const config = {
     'no-empty': 'error',
     'no-eval': 'error',
     'no-new-wrappers': 'error',
+    'no-restricted-globals': [
+      'error',
+      ...['blur', 'close', 'focus', 'length', 'name', 'parent', 'self', 'stop'].map(name => ({
+        name,
+        message: `"${name}" refers to a DOM global. Did you mean to reference a local value instead?`,
+      })),
+    ],
     'no-restricted-properties': [
       'error',
       { object: 'describe', property: 'only', message: 'describe.only should only be used during test development' },
@@ -204,7 +211,6 @@ const config = {
     // permanently disable because we disagree with these rules
     'import/prefer-default-export': 'off',
     'no-await-in-loop': 'off', // contrary to rule docs, awaited things often are NOT parallelizable
-    'no-restricted-globals': 'off', // airbnb bans isNaN in favor of Number.isNaN which is unavailable in IE 11
     'react/jsx-props-no-spreading': 'off',
     'react/prop-types': 'off',
 

diff --git a/packages/monaco-editor/src/configureEnvironment.ts b/packages/monaco-editor/src/configureEnvironment.ts
@@ -10,6 +10,7 @@ export interface IMonacoConfig {
   crossDomain?: boolean;
 }
 
+// eslint-disable-next-line no-restricted-globals
 const globalObj = (typeof self !== 'undefined' ? self : typeof window !== 'undefined' ? window : {}) as Window & {
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
   MonacoEnvironment?: any;

diff --git a/packages/react-next/.eslintrc.json b/packages/react-next/.eslintrc.json
@@ -3,9 +3,6 @@
   "extends": ["../office-ui-fabric-react/.eslintrc"],
   "root": true,
   "rules": {
-    "@typescript-eslint/no-explicit-any": "error",
-    // Disable until issues are dealt with
-    "react-hooks/exhaustive-deps": "off",
-    "react-hooks/rules-of-hooks": "off"
+    "@typescript-eslint/no-explicit-any": "error"
   }
 }
diff --git a/packages/react-next/RELEASE_NOTES.md b/packages/react-next/RELEASE_NOTES.md
@@ -2,17 +2,9 @@
 
 ## Breaking changes
 
-### Beak
+### Calendar
 
-- Removed empty `IBeak` interface
-- Removed `componentRef` prop
-
-### SpinButton
-
-- Simplified props to `ISpinButtonStyles` to include only the parts of the component to bring inline with
-  other components.
-- Replaced `getClassNames` legacy prop with `styles` prop to bring component consistent to other components
-  and improve cachability of internal styles.
+TODO: Diff of OUFR vs date-time Calendar
 
 ### Checkbox
 
@@ -24,10 +16,27 @@
 ### Coachmark
 
 - Removed `isBeaconAnimating` and `isMeasured` style props
+- Beak:
+  - Removed empty `IBeak` interface
+  - Removed `componentRef` prop
+
+### DatePicker
+
+TODO: Diff of OUFR vs date-time DatePicker
 
 ### Pivot
 
 - Removed deprecated and redundant props from v7, including: `initialSelectedKey` and `defaultSelectedIndex`. Use `selectedKey` or `defaultSelectedKey` to define the selected tab, and provide `itemKey` on pivot item children.
+  - TODO: enumerate all removed props
+
+### Slider
+
+TODO: document any API or functionality changes
+
+### SpinButton
+
+- Simplified props to `ISpinButtonStyles` to include only the parts of the component to bring inline with other components.
+- Replaced `getClassNames` legacy prop with `styles` prop to bring component consistent to other components and improve cachability of internal styles.
 
 ### Others
 
@@ -45,7 +54,7 @@
 
 - Updated enums to string union type: `PivotLinkFormat`, `PivotLinkSize`. (#13370)
 
-## Changes worth callout
+## Other notable changes
 
 - `styles` prop backward compat solution.
 - css variables and IE 11 solution.
diff --git a/packages/react-next/src/components/Callout/CalloutContent.base.tsx b/packages/react-next/src/components/Callout/CalloutContent.base.tsx
@@ -28,7 +28,7 @@ import {
 import { Popup } from '../../Popup';
 import { classNamesFunction } from '../../Utilities';
 import { AnimationClassNames } from '../../Styling';
-import { useMergedRefs, useAsync } from '@uifabric/react-hooks';
+import { useMergedRefs, useAsync, useConst } from '@uifabric/react-hooks';
 
 const ANIMATIONS: { [key: number]: string | undefined } = {
   [RectangleEdge.top]: AnimationClassNames.slideUpIn10,
@@ -142,7 +142,7 @@ function useBounds(
       cachedBounds.current = currentBounds;
     }
     return cachedBounds.current;
-  }, [bounds, minPagePadding, target]);
+  }, [bounds, minPagePadding, target, targetRef, targetWindowRef]);
 
   return getBounds;
 }
@@ -174,6 +174,12 @@ function useMaxHeight(
     } else if (hidden) {
       setMaxHeight(undefined);
     }
+    // TODO: check with Michael
+    // - Missing dependencies: coverTarget, directionalHintFixed, isBeakVisible, maxHeight
+    //   (and immutable values async, targetRef)
+    // - "Mutable values like 'targetRef.current' aren't valid dependencies because mutating them
+    //   doesn't re-render the component"
+    // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [targetRef.current, gapSpace, beakWidth, directionalHint, getBounds, hidden]);
 
   return maxHeight;
@@ -215,6 +221,9 @@ function useHeightOffset({ finalHeight, hidden }: ICalloutProps, calloutElement:
     if (!hidden) {
       setHeightOffsetEveryFrame();
     }
+    // TODO: check with Michael
+    // (missing dep on setHeightOffsetEveryFrame)
+    // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [finalHeight, hidden]);
 
   return heightOffset;
@@ -275,6 +284,10 @@ function usePositions(
 
       return () => async.cancelAnimationFrame(timerId);
     }
+    // TODO: check with Michael
+    // missing deps finalHeight, getBounds, onPositioned, positions, props, target
+    // (and immutable values async, calloutElement, hostElement, targetRef)
+    // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [hidden, directionalHint]);
 
   return positions;
@@ -289,16 +302,21 @@ function useAutoFocus(
   calloutElement: React.RefObject<HTMLDivElement>,
 ) {
   const async = useAsync();
+  const hasPositions = !!positions;
   React.useEffect(() => {
-    if (!hidden && setInitialFocus && positions && calloutElement.current) {
+    if (!hidden && setInitialFocus && hasPositions && calloutElement.current) {
       const timerId = async.requestAnimationFrame(
         () => focusFirstChild(calloutElement.current!),
         calloutElement.current,
       );
 
       return () => async.cancelAnimationFrame(timerId);
     }
-  }, [hidden, !!positions]);
+    // TODO: check with Michael
+    // missing dep on setInitialFocus
+    // (and immutable values async, calloutElement)
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [hidden, hasPositions]);
 }
 
 /**
@@ -314,55 +332,56 @@ function useDismissHandlers(
   const isMouseDownOnPopup = React.useRef(false);
   const async = useAsync();
 
-  const mouseDownOnPopup = () => {
-    isMouseDownOnPopup.current = true;
-  };
-
-  const mouseUpOnPopup = () => {
-    isMouseDownOnPopup.current = false;
-  };
+  const mouseDownHandlers = useConst([
+    () => {
+      isMouseDownOnPopup.current = true;
+    },
+    () => {
+      isMouseDownOnPopup.current = false;
+    },
+  ] as const);
 
-  const dismissOnScroll = (ev: Event) => {
-    if (positions && !preventDismissOnScroll) {
-      dismissOnClickOrScroll(ev);
-    }
-  };
+  React.useEffect(() => {
+    const dismissOnScroll = (ev: Event) => {
+      if (positions && !preventDismissOnScroll) {
+        dismissOnClickOrScroll(ev);
+      }
+    };
 
-  const dismissOnResize = (ev: Event) => {
-    if (!preventDismissOnResize) {
-      onDismiss?.(ev);
-    }
-  };
+    const dismissOnResize = (ev: Event) => {
+      if (!preventDismissOnResize) {
+        onDismiss?.(ev);
+      }
+    };
 
-  const dismissOnLostFocus = (ev: Event) => {
-    if (!preventDismissOnLostFocus) {
-      dismissOnClickOrScroll(ev);
-    }
-  };
+    const dismissOnLostFocus = (ev: Event) => {
+      if (!preventDismissOnLostFocus) {
+        dismissOnClickOrScroll(ev);
+      }
+    };
 
-  const dismissOnClickOrScroll = (ev: Event) => {
-    const target = ev.target as HTMLElement;
-    const isEventTargetOutsideCallout = hostElement.current && !elementContains(hostElement.current, target);
+    const dismissOnClickOrScroll = (ev: Event) => {
+      const target = ev.target as HTMLElement;
+      const isEventTargetOutsideCallout = hostElement.current && !elementContains(hostElement.current, target);
 
-    // If mouse is pressed down on callout but moved outside then released, don't dismiss the callout.
-    if (isEventTargetOutsideCallout && isMouseDownOnPopup.current) {
-      isMouseDownOnPopup.current = false;
-      return;
-    }
+      // If mouse is pressed down on callout but moved outside then released, don't dismiss the callout.
+      if (isEventTargetOutsideCallout && isMouseDownOnPopup.current) {
+        isMouseDownOnPopup.current = false;
+        return;
+      }
 
-    if (
-      (!targetRef.current && isEventTargetOutsideCallout) ||
-      (ev.target !== targetWindowRef.current &&
-        isEventTargetOutsideCallout &&
-        (!targetRef.current ||
-          'stopPropagation' in targetRef.current ||
-          (target !== targetRef.current && !elementContains(targetRef.current as HTMLElement, target))))
-    ) {
-      onDismiss?.(ev);
-    }
-  };
+      if (
+        (!targetRef.current && isEventTargetOutsideCallout) ||
+        (ev.target !== targetWindowRef.current &&
+          isEventTargetOutsideCallout &&
+          (!targetRef.current ||
+            'stopPropagation' in targetRef.current ||
+            (target !== targetRef.current && !elementContains(targetRef.current as HTMLElement, target))))
+      ) {
+        onDismiss?.(ev);
+      }
+    };
 
-  React.useEffect(() => {
     // This is added so the callout will dismiss when the window is scrolled
     // but not when something inside the callout is scrolled. The delay seems
     // to be required to avoid React firing an async focus event in IE from
@@ -381,15 +400,43 @@ function useDismissHandlers(
         };
       }
     }, 0);
+    // TODO: check with Michael
+    // missing deps on onDismiss, positions, preventDismissOnLostFocus, preventDismissOnResize, preventDismissOnScroll
+    // (and immutable values async, hostElement, targetRef, targetWindowRef
+    // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [hidden]);
 
-  return [mouseDownOnPopup, mouseUpOnPopup] as const;
+  return mouseDownHandlers;
 }
 
 export const CalloutContentBase = React.memo(
   React.forwardRef((propsWithoutDefaults: ICalloutProps, forwardedRef: React.Ref<HTMLDivElement>) => {
     const props = getPropsWithDefaults(DEFAULT_PROPS, propsWithoutDefaults);
 
+    const {
+      styles,
+      style,
+      ariaLabel,
+      ariaDescribedBy,
+      ariaLabelledBy,
+      className,
+      isBeakVisible,
+      children,
+      beakWidth,
+      calloutWidth,
+      calloutMaxWidth,
+      finalHeight,
+      hideOverflow = !!finalHeight,
+      backgroundColor,
+      calloutMaxHeight,
+      onScroll,
+      // eslint-disable-next-line deprecation/deprecation
+      shouldRestoreFocus = true,
+      target,
+      hidden,
+      onLayerMounted,
+    } = props;
+
     const hostElement = React.useRef<HTMLDivElement>(null);
     const calloutElement = React.useRef<HTMLDivElement>(null);
     const rootRef = useMergedRefs(hostElement, forwardedRef);
@@ -410,35 +457,16 @@ export const CalloutContentBase = React.memo(
     useAutoFocus(props, positions, calloutElement);
 
     React.useEffect(() => {
-      if (!props.hidden) {
-        props.onLayerMounted?.();
+      if (!hidden) {
+        onLayerMounted?.();
       }
-    }, [props.hidden]);
+      // eslint-disable-next-line react-hooks/exhaustive-deps -- should only run if hidden changes
+    }, [hidden]);
+
     // If there is no target window then we are likely in server side rendering and we should not render anything.
     if (!targetWindowRef.current) {
       return null;
     }
-    const {
-      styles,
-      style,
-      ariaLabel,
-      ariaDescribedBy,
-      ariaLabelledBy,
-      className,
-      isBeakVisible,
-      children,
-      beakWidth,
-      calloutWidth,
-      calloutMaxWidth,
-      finalHeight,
-      hideOverflow = !!finalHeight,
-      backgroundColor,
-      calloutMaxHeight,
-      onScroll,
-      // eslint-disable-next-line deprecation/deprecation
-      shouldRestoreFocus = true,
-      target,
-    } = props;
 
     const getContentMaxHeight: number | undefined = maxHeight ? maxHeight + heightOffset : undefined;
     const contentMaxHeight: number | undefined =